Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preparation for deprecating reactions' user metadata #1556

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Preparation for deprecating reactions' user metadata #1556

wants to merge 9 commits into from

Conversation

Niloth-p
Copy link
Collaborator

What does this PR do, and why?

  • Prepares for the deprecation of user metadata from messages' reactions and reaction events
    • Supports both user_id (ZFL>=2) and user["id"] for messages' reactions (with added test cases)
    • Supports both user_id (ZFL>=2) and user["user_id"] for reaction events (with added test cases)
    • Introduces a common get_user_id_from_reaction() for handling the different possible fields
    • Switches to using _all_users_by_id["user_id"] to remove dependency on `reaction["user"]["full_name"]
    • Improves the ReactionEvent schema to support both versions
  • Fixes a bug in reaction event handling (remove op), and adds tests covering the missing cases
  • Refactors the reactions' functions a bit before the above changes

External discussion & connections

  • Discussed in #zulip-terminal in reaction metadata
  • Fully fixes #
  • Partially fixes issue #
  • Builds upon previous unmerged work in PR #
  • Is a follow-up to work in PR #
  • Requires merge of PR #
  • Merge will enable work on #

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

@Niloth-p
refactor: model/views: Simplify functions handling message reactions.
4765327 
- 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.
@Niloth-p
api_types/model: Add support for user_id in reaction events.
d9cd5e5
@Niloth-p
test_model: Align reactions schema with Get Message API endpoint.
e037dc4 
The `reaction_event_index_factory` was previously generating reactions
based on the Reaction Event schema.
@Niloth-p
bugfix: model: Remove the reaction corresponding to the reaction event.
f7e56c9 
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.
@Niloth-p
model: Convert reaction events to message reactions schema.
911f8ee
@Niloth-p
model: Add get_user_id_from_reaction for messages and reaction events.
35d5bee 
In preparation for deprecation of `user` field from reactions.

Tests added.
@Niloth-p
test_model: Add user_id test cases for reaction factories.
6e2f98c 
- reaction_event_factory
- reaction_event_index_factory

To test the handling of reaction events conforming to the reactions
schema post ZFL 2.
@Niloth-p
test_model: Add varied test cases for testing reaction event handling.
198e8f3
@Niloth-p
messages/views: Prepare for the deprecation of user metadata.
005dabb 
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.
@zulipbot zulipbot added the size: XL [Automatic label added by zulipbot] label Nov 18, 2024
@Niloth-p Niloth-p changed the title Reactions metadata/pr Preparation for deprecating reactions' user metadata Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
size: XL [Automatic label added by zulipbot]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Niloth-p @zulipbot