FIX: Display assignments in user menu properly #507
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Flink
force-pushed
the
loic-fix-user-menu
branch
from
615d464 to
bc44425
Compare
Flink
force-pushed
the
loic-fix-user-menu
branch
from
3874e57 to
4317de9
Compare
Flink
force-pushed
the
loic-fix-user-menu
branch
2 times, most recently
from
722951a to
6503fc7
Compare
Flink
force-pushed
the
loic-fix-user-menu
branch
from
288c28d to
6125595
Compare
|
I'll wave to @nlalonde / @heymarkreeves for a review when they have space 👋 |
nlalonde
approved these changes
Oct 30, 2023
Flink
force-pushed
the
loic-fix-user-menu
branch
3 times, most recently
from
7824f24 to
2f998ab
Compare
Currently, we display a mix of topics and notifications in the user menu assignments tab. This has a number of issues like having to maintain hard to understand code instead of simply relying on the notifications system we have, we can’t display more than one assignment per topic and it’s not clear if an assignment is for a topic or a post nor if it’s a group assignment or an individual one. This patch addresses those issues by relying on the notifications system we’re using for most of the other user menu tabs instead of a custom implementation. This led to some heavy refactoring but it was worthwhile as things are a bit more normalized and easier to reason about. Instead of serializing topics with different attributes to access various assignments, we now simply return a notification for each existing assignment. The UI changed a bit: tooltips are now explaining if the assignment is for a topic or a post and if it’s for an individual or a group. Icons are also properly set (so no more individual icon for group assignments) and the assigned group name is displayed. The background jobs signatures changed a bit (`assignment_id` is now needed for the unassign job) so when deploying this patch, it’s expected to have some jobs failing. That’s why a post-migration has been written to handle the creation of missing notifications and the deletion of extra notifications too.
Flink
force-pushed
the
loic-fix-user-menu
branch
from
2f998ab to
b75a0d3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we display a mix of topics and notifications in the user menu assignments tab. This has a number of issues like having to maintain hard to understand code instead of simply relying on the notifications system we have, we can’t display more than one assignment per topic and it’s not clear if an assignment is for a topic or a post nor if it’s a group assignment or an individual one.
This PR addresses those issues by relying on the notifications system we’re using for most of the other user menu tabs instead of a custom implementation. This led to some heavy refactoring but it was worthwhile as things are a bit more normalized and easier to reason about. Instead of serializing topics with different attributes to access various assignments, we now simply return a notification for each existing assignment.
The UI changed a bit: tooltips are now explaining if the assignment is for a topic or a post and if it’s for an individual or a group. Icons are also properly set (so no more individual icon for group assignments) and the assigned group name is displayed.
The background jobs signatures changed a bit (
assignment_idis now needed for the unassign job) so when deploying this patch, it’s expected to have some jobs failing. That’s why a post-migration has been written to handle the creation of missing notifications and the deletion of extra notifications too.