-
Notifications
You must be signed in to change notification settings - Fork 1
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
feature: user-friendly page for closed supervisors reviews #595
Conversation
{% endblocktrans %} | ||
</p> | ||
</div> | ||
{% include "reviews/review_detail_sidebar.html" %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to add the review sidebar to the right of the main content instead of the left as we do in other places.
The main content is way more important for this page, and I felt that having the sidebar on the left distracted from the main content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I have some minor suggestions for textual things, which are more so nicer imo, but I found no real textual errors or anything. Documentation on the new code is very nice and it all looks clear and clean.
locale/en/LC_MESSAGES/django.po
Outdated
msgid "" | ||
"Je hebt deze aanvraag al beoordeeld. Controleer of dit inderdaad de aanvraag " | ||
"is die je wou beoordelen. Op <a href=\"%(my_supervised_url)s\">deze pagina</" | ||
"a> vind je alle aanvragen waarvan je eindverantwoordelijke bent." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ik zou zeggen:
"is die je wilde beoordelen. Op <a href=\"%(my_supervised_url)s\">deze pagina</"
"a> vind je alle aanvragen waarvan je de eindverantwoordelijke bent."
"toevoegen/aanpassen" | ||
msgstr "" | ||
"This application has already been assessed, so you cannot add/adjust your " | ||
"assessment" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
misschien add/edit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking back at it, I'd say `add/change'?
Addresses #594
This PR adds a page supervisors are redirected to if they access a decision URL for a proposal they've already reviewed. The issue talks about a similar page for committee reviews, but looking at the code I decided to just redirect them to the review detail page instead. In that case, they do get a message using the messages framework informing them the review has been concluded.
As part of this change, I also refactored
UserAllowedMixin
into a decision-only mixin (it was only used for decisions nowadays). I also moved the access check to the dispatch method instead; that way, we can still retrieve the object. This was needed, as I needed the actual object to properly redirect users and the previous approach blocked object retrieval.Another small refactor: previously, the success URL was decided by looking at the user making the decision; I decided to look at the review type instead as that felt more consistent in behavior
As always, please double check the texts I added. Even more so for the translations,
as I was very lazy