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.

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

Fix user label disappearing on global feedback update #5272

Merged
merged 3 commits into from
Jan 9, 2024

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Jan 5, 2024

Closes #5252

@jorg-vr jorg-vr requested a review from a team as a code owner January 5, 2024 09:08
@jorg-vr jorg-vr requested review from bmesuere and chvp and removed request for a team January 5, 2024 09:08
@jorg-vr jorg-vr self-assigned this Jan 5, 2024
@jorg-vr jorg-vr added the bug Something isn't working label Jan 5, 2024
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

Can you explain a bit more? This feels like a hack to me.

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Jan 5, 2024

When the all scores are updated (by clicking the top level thumbs up or thumbs down) the whole score card is rerendered. (the html is replaced using javascript)

This card also contains the users labels. But the view file expects these to be present as a variable.

In the show function this variable is defined in the controller. But in the js response on update the same view is reused, but the variable is never defined. Thus the view simply did not draw any labels.

Another option would be to move the function to fetch the user labels from the controller to the view file. This way the definition would only be written once.
But it is unclear to me which best practice takes precedence here: keeping the logic in the controller or avoiding code duplication.

@bmesuere
Copy link
Member

bmesuere commented Jan 8, 2024

In this case, I would:

  • remove @user_labels as a separate variable in both actions and just fetch it from the view since it is part of the feedback object
  • add a shorthand to the model to directly fetch the labels

@jorg-vr jorg-vr requested a review from bmesuere January 9, 2024 09:01
@jorg-vr jorg-vr merged commit d4bfbf2 into main Jan 9, 2024
13 checks passed
@jorg-vr jorg-vr deleted the fix/labels-disappear branch January 9, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User label in grading module sometimes disappears
3 participants