Add support for 'message_changed' subtype for 'message' events

[cchadowitz]

I had a scenario where I wanted the slack-machine to process message events for message_changed subtypes. The issue is that when the event subtype is message_changed, the text and other relevant info end up in a message object within the message event object, e.g. from https://api.slack.com/events/message/message_changed

{
	"type": "message",
	"subtype": "message_changed",
	"hidden": true,
	"channel": "C2147483705",
	"ts": "1358878755.000001",
	"message": {
		"type": "message",
		"user": "U2147483697",
		"text": "Hello, world!",
		"ts": "1355517523.000005",
		"edited": {
			"user": "U2147483697",
			"ts": "1358878755.000001"
		}
	}
}

This PR adds support for that use case by adding an additional optional parameter to the @listen_to and @respond_to decorators (default False) called handle_changed_message that when set to True, massages the data in the message events with subtype message_changed so that they get correctly processed and passed along to plugin functions that use the handle_changed_message=True parameter in the decorators.

In addition, I added a subtype property to the Message class so that plugin methods can easily check the subtype of the incoming message and process it appropriately (e.g. if you want to compare the original message to the edited message).

I considered adding new decorators for this purpose but went with this approach instead for minimal code duplication. Happy to tweak if desired, though, just let me know!

[cchadowitz]

I think these changes made it in another way, feel free to re-open if not (or I will if it seems like not).

[cchadowitz]

My mistake, they're not yet. Fixed merge conflicts and added one additional fix.

[cchadowitz]

There is one failing test and I left a comment. Can you fix the test and address my comment?

I replied to your comment. I admit I'm not 100% sure what's going on with the test. It looks like a few failed with the same error:

  >           if listener['handle_changed_message'] and 'subtype' in event and event['subtype'] == 'message_changed':
  E           KeyError: 'handle_changed_message'

What's odd is that I can't see why this would be happening as my PR includes the change to the listen_to decorator here https://github.com/DonDebonair/slack-machine/pull/465/files#diff-f17595faaa2494b64dd6f1ddc3720cf543d78145d16c4feb6673bb332120993dL33 which adds the missing key handle_changed_message with a default value of False.

Any idea what's going on here? Thanks!

[DonDebonair]

There is one failing test and I left a comment. Can you fix the test and address my comment?

I replied to your comment. I admit I'm not 100% sure what's going on with the test. It looks like a few failed with the same error:

  >           if listener['handle_changed_message'] and 'subtype' in event and event['subtype'] == 'message_changed':
  E           KeyError: 'handle_changed_message'

What's odd is that I can't see why this would be happening as my PR includes the change to the listen_to decorator here #465 (files) which adds the missing key handle_changed_message with a default value of False.

Any idea what's going on here? Thanks!

Yeah, so the decorators mostly do one thing: register metadata on the plugin functions. When slack-machine is started, it tries to dynamically load all plugins and their functions and reads that metadata that was added by the decorators and builds up a dictionary of plugin actions. This happens here

You need to add your new setting to the event_handler dict for each plugin function.

Another thing that I noticed in the way you add the handle_changed_message setting in the decorator, is that you append to a list. You probably copied that from the way the rexeges are added, but it makes more sense to group the regex + handle_changed_message together in their own dict so that they can be read in the _register_plugin_actions() function I just linked.

It's all a bit complicated, but you'll have to find your way through it by diving into the source code and tests, to fully understand what is happening.

[cchadowitz]

I see - I admit most of the changes I made in this PR are quite old (over a year in fact I realized!) so I'm definitely rusty on what I had done but it looks like I did actually do exactly what you described (except for the last point you mentioned) in core.py here: https://github.com/DonDebonair/slack-machine/pull/465/commits/40cee1f4d5ce32c3189a1c58cec7a9c00463549b

To your last point, I'm not 100% certain why it makes more sense to group the regex and handle_changed_message together as they are separate pieces? Do you mean that handle_changed_message doesn't need to be a list and can just be a single bool value in the metadata?

Sorry for the back-and-forth! Between the fact that I haven't looked at this in great detail in over a year combined with not being very familiar with python decorators this is definitely a bit more complicated than I originally thought 😅

[DonDebonair]

Ah I see now why those tests fail. test_dispatch.py uses a fixture to generate plugin actions to test against, so it bypasses the logic in core.py. If you update the fixture, the tests should work.

As for my comment on grouping together regex and handle_changed_message: in your current implementation you need to zip together the regex and handle_changed_message. That seems a bit ugly. That's why I suggested grouping them together.

You'll still need a list to hold the grouped regex+handle_changed_message. The reason I'm using lists instead of just a string (in case of regex) or a bool (in case of handle_changed_message) is because the decorator might be applied multiple times on one function. In that case we're dealing with the function being a handler for multiple regexes (or with your addition: multiple regex+handle_changed_message combinations).

[cchadowitz]

I think I understand! I just pushed a change to fix the failing tests and added a new test for additional coverage. I also reworked the bit involving regex + handle_changed_message, let me know if that's roughly what you meant/had in mind.

And we'll see if any tests fail with all of these new changes, too! :) Edit: spoke too soon....

[cchadowitz]

Finally sorted through all the failing tests :) Feel free to let me know if what I have is what you had in mind!

[github-actions[bot]]

Test Results

    4 files      4 suites   15s :stopwatch:   57 tests   57 :heavy_check_mark: 0 :zzz: 0 :x: 228 runs  228 :heavy_check_mark: 0 :zzz: 0 :x:

Results for commit 0c14cf1c.

[codecov[bot]]

Codecov Report

Merging #465 (0c14cf1) into main (76e7866) will increase coverage by 0.45%. The diff coverage is 90.90%.

@@            Coverage Diff             @@
##             main     #465      +/-   ##
==========================================
+ Coverage   70.67%   71.12%   +0.45%     
==========================================
  Files          22       22              
  Lines         931      949      +18     
==========================================
+ Hits          658      675      +17     
- Misses        273      274       +1     

Impacted Files	Coverage Δ
machine/plugins/base.py	57.26% <40.00%> (-0.78%)	:arrow_down:
machine/core.py	70.77% <100.00%> (+0.38%)	:arrow_up:
machine/dispatch.py	90.90% <100.00%> (+3.40%)	:arrow_up:
machine/plugins/decorators.py	73.68% <100.00%> (+1.77%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us.

[DonDebonair]

Hey @cchadowitz-pf! We're almost there! The solution for combining regex and handle_changed_message in the decorators was not exactly what I had in mind. I also noticed that somehow the git history in your PR was somehow messed up, probably due to all the changes that happend in the repo between when you started your PR and now.

So what I did, is create a patch from your changes, and apply it on a new branch from main (with your name + email as the commit author, so you get credit!). On top of that I added a commit with the changes I'd like to see. You can see it in this PR.

I don't have time to test it right now, to see if it still does what you intend it to do. Could you maybe test my PR? If it works, I'll merge it and cut a new release!

[DonDebonair]

Hey @cchadowitz-pf this was merged in #594 and published as v0.28.1

I will soon also port it to the async version