Skip to content

FIX: Ensure client-side reviewable claiming data is set correctly #33405

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

FIX: Ensure client-side reviewable claiming data is set correctly #33405

wants to merge 1 commit into from
+4 −1

Conversation

pento
Copy link
Member

@pento pento commented Jul 1, 2025

✨ What's This?

ReviewableClaimedTopic has an onClaim arg, which is used by the calling components to locally mutate the value of reviewable.claimed_by.

This was being incorrectly mutated to a User model, when it in fact needs to match the ReviewableClaimedTopicSerializer output.

👑 Testing

Claim and unclaim reviewables, confirm that:

  • The UI updates correctly as soon as you perform the claim/unclaim (client side updates are correct).
  • The UI is displayed correctly after reloading the page (server side updates are correct).

@pento pento self-assigned this Jul 1, 2025
@tgxworld
Copy link
Contributor

tgxworld commented Jul 1, 2025

Do we need a regression test here since not being able to claim a reviewable seems to be quite bad? 🤔

@pento
Copy link
Member Author

pento commented Jul 1, 2025

Claiming a reviewable works, since it performs the ajax/save call before the onClaim() call. The issue is that it doesn't update the client-side store correctly, so the UI didn't update properly until the store was refreshed from the server.

tgxworld
tgxworld approved these changes Jul 1, 2025
View reviewed changes
Copy link
Contributor

@tgxworld tgxworld left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally do think it is worth adding a test here for the happy path but don't think it needs to be blocking.

Approving 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgxworld tgxworld tgxworld approved these changes

Assignees

@pento pento

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pento @tgxworld