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

Adds warning when setting same quiz to class multiple times #1071

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

sjd210
Copy link
Contributor

@sjd210 sjd210 commented Aug 21, 2024

Previously on re-submitting an active test, a toast would display a back-end warning "You cannot reassign a test until the due date has passed."

This change makes it fail earlier (on the front-end), displaying red warning text below the "...group(s)" input and disabling the "Set Test" button.


I am making use of a top-level let as a workaround to stop polling the back-end on every change to the Set Tests modal (while avoiding dreaded React hook nonsense - otherwise useEffect would be lovely here). Since the value only changes on successfully setting a test, it does not need a re-render so this should be fine - however I'm very open to suggestions for another solution.

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 36.83%. Comparing base (24b1e2c) to head (328d4fa).
Report is 236 commits behind head on master.

Files with missing lines Patch % Lines
...pp/components/elements/modals/QuizSettingModal.tsx 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1071      +/-   ##
==========================================
- Coverage   36.88%   36.83%   -0.06%     
==========================================
  Files         426      426              
  Lines       18803    18872      +69     
  Branches     5568     5582      +14     
==========================================
+ Hits         6936     6952      +16     
- Misses      11828    11881      +53     
  Partials       39       39              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sjd210 sjd210 marked this pull request as ready for review August 22, 2024 08:30
Copy link
Contributor

@mlt47 mlt47 left a comment

Choose a reason for hiding this comment

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

This works and the implementation is along the right lines.
However, I believe that the RTK Query hook is actually doing what you want from it by default, so we should be able to simplify the implementation by switch to just using it! :)

src/app/components/elements/modals/QuizSettingModal.tsx Outdated Show resolved Hide resolved
src/app/components/elements/modals/QuizSettingModal.tsx Outdated Show resolved Hide resolved
@@ -46,6 +47,8 @@ interface QuizSettingModalProps {
feedbackMode?: QuizFeedbackMode | null;
}

let allQuizAssignments: QuizAssignmentDTO[] | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

As we have this information (perhaps retrieved in a different way given other comments) it might be useful to alter groupOptions to add " (already assigned)" to the group label in the drop-down so that the user gets a warning even earlier. It is possible that this will look overcrowded in which case there might be an alternative solution that is nicer - I don't know if the component supports disabled options, for example.

Copy link
Contributor Author

@sjd210 sjd210 Aug 28, 2024

Choose a reason for hiding this comment

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

I think this is a good call, and also that it works best with items being disabled in the StyledSelect.

I initially had some issues with disabled items still being focused by default (annoyingly React has been working on a solution to this for a while), but the change to options in styles solves that in the scope of this page, so I'm pretty happy with it all now.

@sjd210 sjd210 requested a review from mlt47 August 28, 2024 16:16
@mlt47 mlt47 merged commit 7bb3ab1 into master Sep 10, 2024
4 checks passed
@mlt47 mlt47 deleted the improvement/no-resetting-active-quiz branch September 10, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants