Preparation for deprecating reactions' user metadata #1556
Conversation
- Simplified reaction toggling in `toggle_message_reaction()` - Simplified reaction event handling in `_handle_reaction_event()` - Simplified reaction agreement command logic Removed comments for self-explanatory code.
The `reaction_event_index_factory` was previously generating reactions based on the Reaction Event schema.
When multiple users shared an emoji reaction, the prior implementation did not account for individual user IDs, leading to buggy handling of 'remove reaction' events.
In preparation for deprecation of `user` field from reactions. Tests added.
- reaction_event_factory - reaction_event_index_factory To test the handling of reaction events conforming to the reactions schema post ZFL 2.
The "user" field is to be deprecated from messages' reactions and reaction events in the API. Add support for `user_id` in MsgInfoView, by using the recently introduced `get_user_id_from_reaction()`. Remove dependence on reaction["user"]["full_name"] by instead using `_all_users_by_id` with the "user_id". Tests updated.
Niloth-p
changed the title
There was a problem hiding this comment.
Thanks for working on this @Niloth-p
At first glance, the get_user_id_from_reaction() seemed unnecessary to me but on closer look, multiple usages of the same logic make the function a necessity. The coverage of cases is more vast than the impact that I had expected. Overall, this worked as expected across all the use cases that I tried for.
I think the commit structure can be changed. You can squash the initial commits which work towards simplifying/switching to the new API response. Then the next commit can be the patch providing the new function of get_user_id_from_reaction() and its test function. Then you may change all implementations of the older response and add the function calls wherever necessary.
Apart from this, I don't have any other changes to suggest for this PR.
| if ( | ||
| reaction["emoji_code"] == event["emoji_code"] | ||
| and reaction_user_id == event_user_id | ||
| ): | ||
| message["reactions"].remove(reaction) | ||
| break |
There was a problem hiding this comment.
Feature level 3.0 is old enough that we could drop support for it. But I guess this is OK.
|
I did a fairly careful read, and this seems like a good PR to me, and I think it makes sense to merge and fix-forward from here as needed. It's sadly a bit of wasted effort -- we could just process |
|
Thank you! |
What does this PR do, and why?
user_id(ZFL>=2) anduser["id"]for messages' reactions (with added test cases)user_id(ZFL>=2) anduser["user_id"]for reaction events (with added test cases)get_user_id_from_reaction()for handling the different possible fields_all_users_by_id["user_id"]to remove dependency on `reaction["user"]["full_name"]ReactionEventschema to support both versionsExternal discussion & connections
reaction metadataHow did you test this?
Self-review checklist for each commit
Visual changes