feat: add support for 'message_changed' subtype for 'message' events

[DonDebonair]

@cchadowitz-pf this is my version of your proposed changes

[github-actions[bot]]

Test Results

    4 files  ±  0      4 suites  ±0   22s :stopwatch: -1s   93 tests +  3    93 :heavy_check_mark: +  3  0 :zzz: ±0  0 :x: ±0  372 runs  +12  372 :heavy_check_mark: +12  0 :zzz: ±0  0 :x: ±0 

Results for commit 566519a6. ± Comparison against base commit 4e402294.

:recycle: This comment has been updated with latest results.

[codecov[bot]]

Codecov Report

Merging #594 (566519a) into main (4e40229) will increase coverage by 0.13%. The diff coverage is 85.18%.

@@            Coverage Diff             @@
##             main     #594      +/-   ##
==========================================
+ Coverage   68.01%   68.14%   +0.13%     
==========================================
  Files          39       39              
  Lines        1885     1899      +14     
==========================================
+ Hits         1282     1294      +12     
- Misses        603      605       +2     

Impacted Files	Coverage Δ
machine/plugins/base.py	57.26% <40.00%> (-0.78%)	:arrow_down:
machine/dispatch.py	89.61% <83.33%> (+2.11%)	:arrow_up:
machine/core.py	70.77% <100.00%> (+0.38%)	:arrow_up:
machine/plugins/decorators.py	72.52% <100.00%> (+0.61%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[DonDebonair]

@cchadowitz-pf wanna test this and see if this does what you want?

[cchadowitz]

Oops sorry I meant to take a look at this and totally forgot. Unfortunately I probably won't have time to test this out till early next week.

Having only taken a very brief look through, it actually looks like this PR is most of what I had, except in the decorators the regex and new param are stored in config instead of params, although params is still referenced elsewhere. I need to take a closer look but is that intended?

[DonDebonair]

That was a mistake on my side. _dispatch_listeners should not refer to params anymore. Will fix that soon

[DonDebonair]

That was a mistake on my side. _dispatch_listeners should not refer to params anymore. Will fix that soon

I fixed this as well @cchadowitz-pf . So as far as I'm concerned, this is good to go. Haven't tested it thoroughly though

[cchadowitz]

Great thanks! I may not be able to get to this today but I'll take a look/test asap.

[DonDebonair]

@cchadowitz-pf did you have time to look at this?

[cchadowitz]

@DonDebonair sorry this slipped last week. Overall it seems to work fine on my system, just one question you may or may not have an answer to: I'd been running a custom branch build of my own in the meantime for many months to handle changed messages until now, and one of my custom plugin handlers used to use msg._msg_event['previous_message']['text'] to identify the difference between the original message and the new (changed) message. I can't seem to find any reference to previous_message in the Slack RTM API docs, though - wondering if you had any familiarity and are aware of whether it used to be present and has since been removed from the Slack API or something else.

Again, doesn't directly affect this PR except that if there is a way to include the previous message that would be quite convenient. Thanks for all your help!

Edit to add: I did find this one reference to previous_message, so at the very least I don't think I was imagining it :)

[DonDebonair]

I can't find any references in the Slack docs to previous_message either. Do you want to figure that out first? Or should we rebase and merge this branch after you've tested it?

edit: the reference you found, is in the Slack Events API, which is different than the RTM API. I've been building an async version of Slack Machine (lives in machine.asyncio) which is completely built on top of the Events API and doesn't use the RTM API. I will debug a bit to see if the previous message is present in the event in case of a message change event

[cchadowitz]

Ah I see - I'd be curious to see what you find. The only reason I was even using previous_message is to avoid processing the same text contents twice in the case of a message changing (i.e. only process the new bits).

[DonDebonair]

It turns out, that in the Events API, the message event contains a previous_message if the subtype is message_changed:

{  
    "token": "ZMBw88SmAGYGpwguEEH4bZ84",  
    "team_id": "TQSD32X16",  
    "api_app_id": "A039QKQ6G1E",  
    "event": {  
        "type": "message",  
        "subtype": "message_changed",  
        "message": {  
            "client_msg_id": "100c2676-edf1-4b7f-ae1f-f4a7872fd051",  
            "type": "message",  
            "text": "hello there edited",  
            "user": "UQEUMSA0K",  
            "team": "TQSD32X16",  
            "edited": {"user": "UQEUMSA0K", "ts": "1661370202.000000"},  
            "blocks": [  
                {  
                    "type": "rich_text",  
                    "block_id": "HrHML",  
                    "elements": [  
                        {"type": "rich_text_section", "elements": [{"type": "text", "text": "hello there edited"}]}  
                    ],  
                }  
            ],  
            "ts": "1661370190.992709",  
            "source_team": "TQSD32X16",  
            "user_team": "TQSD32X16",  
        },  
        "previous_message": {  
            "client_msg_id": "100c2676-edf1-4b7f-ae1f-f4a7872fd051",  
            "type": "message",  
            "text": "hello there",  
            "user": "UQEUMSA0K",  
            "ts": "1661370190.992709",  
            "team": "TQSD32X16",  
            "blocks": [  
                {  
                    "type": "rich_text",  
                    "block_id": "P2e",  
                    "elements": [{"type": "rich_text_section", "elements": [{"type": "text", "text": "hello there"}]}],  
                }  
            ],  
        },  
        "channel": "CQEUMSV7D",  
        "hidden": True,  
        "ts": "1661370202.000500",  
        "event_ts": "1661370202.000500",  
        "channel_type": "channel",  
    },  
    "type": "event_callback",  
    "event_id": "Ev03VDL71C9X",  
    "event_time": 1661370202,  
    "authorizations": [  
        {  
            "enterprise_id": None,  
            "team_id": "TQSD32X16",  
            "user_id": "U038Y1G7745",  
            "is_bot": True,  
            "is_enterprise_install": False,  
        }  
    ],  
    "is_ext_shared_channel": False,  
    "event_context": (  
        "4-eyJldCI6Im1lc3NhZ2UiLCJ0aWQiOiJUUVNEMzJYMTYiLCJhaWQiOiJBMDM5UUtRNkcxRSIsImNpZCI6IkNRRVVNU1Y3RCJ9"  
    ),  
}

But I couldn't find it in the RTM API indeed. So if you still want this feature, I suggest we merge it as is. As mentioned, I'm working on converting Slack Machine to use the Events API and make it asyncio. If we merge your feature, I will incorporate it in the new asyncio version as well. From there, you can see if/how you want to use previous_message somehow as well

[cchadowitz]

Sounds good to me! Thanks for your efforts in this and looking forward to the Events API/asyncio update down the road!