Don't re-set channel and sheet name or channel topic to the same value as before

The puzzle_update_channel_and_sheet function is modified here to
accept an old_puzzle argument in addition to the puzzle argument. By
using this it can decide whether there's any change in the sheet or
channel data before setting it.

This optimization avoids Slack reporting things like:

    Turbot renamed the channel from "hunt-puzzle" to "hunt-puzzle"

Which is obviously just noise.

diff --git a/turbot/interaction.py b/turbot/interaction.py
index 0b2f30eec9324c9ba8689a266d9f3ad683e2fd78..3254f24587508953f7bf19d32905b5ea194dd68d 100644 (file)
--- a/turbot/interaction.py
+++ b/turbot/interaction.py
@@ -229,13 +229,18 @@ def edit_puzzle_submission(turb, payload, metadata):
                 }
             )
 
+    # Get old puzzle from the database (to determine what's changed)
+    old_puzzle = find_puzzle_for_puzzle_id(turb,
+                                           puzzle['hunt_id'],
+                                           puzzle['puzzle_id'])
+
     # Update the puzzle in the database
     turb.table.put_item(Item=puzzle)
 
     # We need to set the channel topic if any of puzzle name, url,
     # state, status, or solution, has changed. Let's just do that
     # unconditionally here.
-    puzzle_update_channel_and_sheet(turb, puzzle)
+    puzzle_update_channel_and_sheet(turb, puzzle, old_puzzle=old_puzzle)
 
     return lambda_ok
 
@@ -682,17 +687,20 @@ def state(turb, body, args):
 
     channel_id = body['channel_id'][0]
 
-    puzzle = puzzle_for_channel(turb, channel_id)
+    old_puzzle = puzzle_for_channel(turb, channel_id)
 
-    if not puzzle:
+    if not old_puzzle:
         return bot_reply(
             "Sorry, the /state command only works in a puzzle channel")
 
-    # Set the state field in the database
+    # Make a copy of the puzzle object
+    puzzle = old_puzzle.copy()
+
+    # Update the puzzle in the database
     puzzle['state'] = args
     turb.table.put_item(Item=puzzle)
 
-    puzzle_update_channel_and_sheet(turb, puzzle)
+    puzzle_update_channel_and_sheet(turb, puzzle, old_puzzle=old_puzzle)
 
     return lambda_ok
 
@@ -706,15 +714,18 @@ def solved(turb, body, args):
     channel_id = body['channel_id'][0]
     user_name = body['user_name'][0]
 
-    puzzle = puzzle_for_channel(turb, channel_id)
+    old_puzzle = puzzle_for_channel(turb, channel_id)
 
-    if not puzzle:
+    if not old_puzzle:
         return bot_reply("Sorry, this is not a puzzle channel.")
 
     if not args:
         return bot_reply(
             "Error, no solution provided. Usage: `/solved SOLUTION HERE`")
 
+    # Make a copy of the puzzle object
+    puzzle = old_puzzle.copy()
+
     # Set the status and solution fields in the database
     puzzle['status'] = 'solved'
     puzzle['solution'].append(args)
@@ -737,7 +748,7 @@ def solved(turb, body, args):
     )
 
     # And update the puzzle's description
-    puzzle_update_channel_and_sheet(turb, puzzle)
+    puzzle_update_channel_and_sheet(turb, puzzle, old_puzzle=old_puzzle)
 
     return lambda_ok
 

diff --git a/turbot/puzzle.py b/turbot/puzzle.py
index 1a36bfb9be433460d850672733c4bfed92d6728e..f0456c6cba4c29d05cf4dcc23ed96953fea5c940 100644 (file)
--- a/turbot/puzzle.py
+++ b/turbot/puzzle.py
@@ -153,51 +153,38 @@ def puzzle_matches_all(puzzle, patterns):
 def puzzle_id_from_name(name):
     return re.sub(r'[^a-zA-Z0-9_]', '', name).lower()
 
-def puzzle_update_channel_and_sheet(turb, puzzle):
-
-    channel_id = puzzle['channel_id']
-    name = puzzle['name']
-    url = puzzle.get('url', None)
-    sheet_url = puzzle.get('sheet_url', None)
-    state = puzzle.get('state', None)
-    status = puzzle['status']
+def puzzle_channel_topic(puzzle):
+    """Compute the channel topic for a puzzle"""
 
     topic = ''
 
-    if status == 'solved':
+    if puzzle['status'] == 'solved':
         topic += "SOLVED: `{}` ".format('`, `'.join(puzzle['solution']))
 
-    topic += name
+    topic += puzzle['name']
 
     links = []
+
+    url = puzzle.get('url', None)
     if url:
         links.append("<{}|Puzzle>".format(url))
+
+    sheet_url = puzzle.get('sheet_url', None)
     if sheet_url:
         links.append("<{}|Sheet>".format(sheet_url))
 
     if len(links):
         topic += "({})".format(', '.join(links))
 
+    state = puzzle.get('state', None)
     if state:
         topic += " {}".format(state)
 
-    # Slack only allows 250 characters for a topic
-    if len(topic) > 250:
-        topic = topic[:247] + "..."
-
-    turb.slack_client.conversations_setTopic(channel=channel_id,
-                                             topic=topic)
+    return topic
 
-    # Rename the sheet to include indication of solved/solution status
-    sheet_name = puzzle['name']
-    if puzzle['status'] == 'solved':
-        sheet_name += " - Solved {}".format(", ".join(puzzle['solution']))
+def puzzle_channel_name(puzzle):
+    """Compute the channel name for a puzzle"""
 
-    turbot.sheets.renameSheet(turb, puzzle['sheet_url'], sheet_name)
-
-    # Finally, rename the Slack channel to reflect the latest name and
-    # the solved status
-    #
     # Note: We don't use puzzle['hunt_id'] here because we're keeping
     # that as a persistent identifier in the database. Instead we
     # create a new ID-like identifier from the current name.
@@ -205,10 +192,58 @@ def puzzle_update_channel_and_sheet(turb, puzzle):
         puzzle['hunt_id'],
         puzzle_id_from_name(puzzle['name'])
     )
+
     if puzzle['status'] == 'solved':
         channel_name += "-solved"
 
-    turb.slack_client.conversations_rename(
-        channel=puzzle['channel_id'],
-        name=channel_name
-    )
+    return channel_name
+
+def puzzle_sheet_name(puzzle):
+    """Compute the sheet name for a puzzle"""
+
+    sheet_name = puzzle['name']
+    if puzzle['status'] == 'solved':
+        sheet_name += " - Solved {}".format(", ".join(puzzle['solution']))
+
+    return sheet_name
+
+def puzzle_update_channel_and_sheet(turb, puzzle, old_puzzle=None):
+
+    channel_id = puzzle['channel_id']
+
+    # Compute the channel topic and set it if it has changed
+    channel_topic = puzzle_channel_topic(puzzle)
+
+    old_channel_topic = None
+    if old_puzzle:
+        old_channel_topic = puzzle_channel_topic(old_puzzle)
+
+    if channel_topic != old_channel_topic:
+        # Slack only allows 250 characters for a topic
+        if len(channel_topic) > 250:
+            channel_topic = channel_topic[:247] + "..."
+        turb.slack_client.conversations_setTopic(channel=channel_id,
+                                                 topic=channel_topic)
+
+    # Compute the sheet name and set it if it has changed
+    sheet_name = puzzle_sheet_name(puzzle)
+
+    old_sheet_name = None
+    if old_puzzle:
+        old_sheet_name = puzzle_sheet_name(old_puzzle)
+
+    if sheet_name != old_sheet_name:
+        turbot.sheets.renameSheet(turb, puzzle['sheet_url'], sheet_name)
+
+    # Compute the Slack channel name and set it if it has changed
+    channel_name = puzzle_channel_name(puzzle)
+
+    old_channel_name = None
+    if old_puzzle:
+        old_channel_name = puzzle_channel_name(old_puzzle)
+
+    if channel_name != old_channel_name:
+        turb.slack_client.conversations_rename(
+            channel=channel_id,
+            name=channel_name
+        )