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

textanswer flagging #1619

Conversation

LucPrestin
Copy link
Collaborator

@LucPrestin LucPrestin commented Aug 16, 2021

Summary:
Adds flags to textanswer reviews to indicate the need for an discussion about a text answer with other staff members

Resolves #1118

Changes:

  • Add TextAnswer.is_flagged boolean field
  • Add buttons to toggle text answer flags
  • Add view to list only flagged text answers
  • enable flagging in the texanswers quick view
  • enable flagging in the textanswers normal view (full, unreviewed and flagged)
  • make all flag buttons yellow
  • fix flag status display
  • update tests to new url
  • add safeguard to textanswer toggling

…on about a text answer with other staff members

- Add TextAnswer.is_flagged boolean field
- Add buttons to toggle text answer flags
- Add view to list only flagged text answers
- enable flagging in the texanswers quick view
- enable flagging in the textanswers normal view (full, unreviewed and flagged)
- make all flag buttons yellow
- fix flag status display
- update tests to new url
- add safeguard to textanswer toggling

co-authored-by: Luc Prestin <40767277+LucPrestin@users.noreply.github.com>
@richardebeling richardebeling linked an issue Aug 16, 2021 that may be closed by this pull request
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

i didn't test the functionality yet, but it looks promising so far.

please note that the evaluations suggested in quick review need to be filtered (#1118 (comment)) and that the issue calls for an overview page for all flagged text answers of a semester.

evap/staff/templates/quick-review.js Outdated Show resolved Hide resolved
evap/staff/views.py Outdated Show resolved Hide resolved
@richardebeling
Copy link
Member

richardebeling commented Apr 12, 2022

Did we forget to properly review this? If so, I'm sorry. There are quite some merge conflicts now, but they should be solvable.

On a quick glance, most of the code looks good. I would, however, try to abstain from "toggle" semantics with values where your local representation might be outdated. Consider this scenario:

Staff user 1 opens the review page. They are interrupted, so they suspend their computer. 90 Minutes later, they open it again, to continue reviewing.
In the mean time, staff user 2 has already flagged the answer. But, since staff user 1 didn't reload the page, they are still shown "not flagged" as state. Staff user 1 now also wants to flag, but the "toggle" at the server actually unflags.

(What makes it even worse here is that user 2 will have no indication, their screen will show "flagged" now -- but even without that: Just always send a "target" state to make your operations idempotent, and we don't have to worry about this)

@niklasmohrin niklasmohrin self-assigned this Aug 1, 2022
@janno42
Copy link
Member

janno42 commented Aug 1, 2022

Will be reimplemented by @niklasmohrin after #1776 is merged

@janno42 janno42 closed this Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Text answer flagging
5 participants