Customizable evaluation results filters #2232
Conversation
jooooosef
changed the title
ybrnr
force-pushed
the
iss1786
branch
4 times, most recently
from
0127464 to
ca11745
Compare
| @@ -567,24 +582,6 @@ def test_invalid_contributor_id(self): | |||
| self.app.get(self.url + "?contributor_id=asd", user=self.manager, status=400) | |||
| self.app.get(self.url + "?contributor_id=1234", user=self.manager, status=404) | |||
|
|
|||
| def test_evaluation_sorting(self): | |||
There was a problem hiding this comment.
that was an accident I added it again
There was a problem hiding this comment.
Note: This is still removed in the current state of your branch.
richardebeling
changed the title
ybrnr
force-pushed
the
iss1786
branch
2 times, most recently
from
9bd5cea to
8aaee00
Compare
ybrnr
force-pushed
the
iss1786
branch
2 times, most recently
from
e77a156 to
73c3a65
Compare
| @@ -199,6 +209,30 @@ def evaluation_detail(request, semester_id, evaluation_id): | |||
|
|
|||
| is_responsible_or_contributor_or_delegate = evaluation.is_user_responsible_or_contributor_or_delegate(view_as_user) | |||
|
|
|||
| general_textanswers = False | |||
There was a problem hiding this comment.
Why is all this new code here necessary now, which part of this is is not covered by the access computation methods above?
There was a problem hiding this comment.
This is for enabling / disabling the buttons on the page (see evap/results/templates/results_evaluation_detail.html).
Since some people can use the general buttons (general_textanswers), the full and ratings of contributor (contributor_textanswers) and some can use the personal button as well (contributor_personal). As per request the buttons should be disabled if they dont have an effect on the results (e.g. bc user is not contributor but reponsible in this eval).
There was a problem hiding this comment.
Ok. I'm afraid that we're duplicating access control logic here, even if it is just for adding a html disabled attribute, it is very likely that we will foget to update this view if we ever want to change the access control logic in can_textanswer_be_seen_by. Optimally, both would reuse the same decision logic.
Maybe it helps if we make this more declarative and use more precise naming, although that doesn't fully resolve the underlying issue.
Would something like this work? (Helper is made up, would need to be added)
buttons_enabled = {
"general_full": view_as_user.is_reviewer or represents_or_is_contributor_or_responsible(view_as_user, evaluation)
"general_ratings": # ...
"contributor_full": # ...
"contributor_ratings": # ...
"contributor_personal": # ...
}
Why is contributor_personal = True if view_as_user is a contributor, independently of what the contribution's textanswer_visibility is, but in the loop for the represented users, it is only set to true if the contribution has TextAnswerVisibility.GENERAL_TEXTANSWERS? It seems to me that both would need to be filtered for the contribution's text answer visibility.
Maybe, the helper we want here is def has_textanswer_visibility(user: UserProfile, evaluation: Evaluation, visibility: TextAnswerVisibility), which would answer the question
Is this user allowed to see text answers with this visibility classification (by being a reviewer, a contributor, a responsible, or by being a representative of a reviewer, contributor or responsible
(There might be better names) (Optimally, can_textanswer_be_seen_by would then also use that.)
@niklasmohrin any ideas on how to solve this nicely, here?
| # redirect to non-public view if there is none because the results have not been published | ||
| if not evaluation.can_publish_rating_results and view == "public": | ||
| view = "full" |
There was a problem hiding this comment.
Why is this no longer needed?
There was a problem hiding this comment.
Previously there was a view called "public" which now would be Genereral textanswers on ratings and contributor textanswers on ratings. But even in a preview, there are textanswers, so a user might want to hide these as well. So we dont want to force the user into a view which is hiding all textanswers.
There was a problem hiding this comment.
You mean "which is showing all textanswers"? Hmm, ok.
@janno42 can you confirm that we no longer want to have this "redirect" here? I see that it made sense in the old system, I'm not sure how exactly we want to handle it in the new system.
| if Contribution.objects.filter( | ||
| contributor=user, | ||
| evaluation=evaluation, |
There was a problem hiding this comment.
| if Contribution.objects.filter( | |
| contributor=user, | |
| evaluation=evaluation, | |
| if evaluation.contributions.filter( | |
| contributor=user, |
There was a problem hiding this comment.
I though the first "evaluation" is from the ORM (so a column in the database) and the second "evaluation" (after the =) is the defined at the beginning with evaluation = get_object_or_404(semester.evaluations, id=evaluation_id, course__semester=semester) and then in the if statement a couple lines earlier with evaluation = get_evaluations_with_course_result_attributes(prefetched)[0]. Is this how it works?
| <div class="row"> | ||
| <div class="col-auto"> | ||
| <div class="btn-switch btn-switch-light my-auto d-print-none"> | ||
| <div class="btn-switch-label">{% translate 'General results' %}</div> |
There was a problem hiding this comment.
when disabled, "Ratings" should be selected/active and its tooltip should be shown on hover.
a different tooltip should be shown for "All" when disabled: "You can't see text answers for general questions in this evaluation."
| <div class="row mt-1"> | ||
| <div class="col-auto"> | ||
| <div class="btn-switch btn-switch-light my-auto d-print-none"> | ||
| <div class="btn-switch-label">{% translate 'Contributor results' %}</div> |
There was a problem hiding this comment.
when disabled, "Ratings" should be selected/active and its tooltip should be shown on hover.
a different tooltip should be shown for the other buttons when disabled: "You can't see text answers for contributor questions because you aren't a contributor."
There was a problem hiding this comment.
We added that ratings is selected when the button group is disabled.
Showing the tooltip is not this easily possible since the buttons are disabled and therefore dont accept any mouse events (inlcuding mouse hover), thus the title is not shown.
I think this could be fixed with some js?
There was a problem hiding this comment.
I think we found a way of having tooltips on disabled buttons in #1931.
…py müssen wir uns nochmal anschauen, da failed noch ein test
…est_tools zusammengeführt, jetzt alles clean
…aber jetzt geht wieder
…ack on what problem we are working atm
made the can_see_general_textanswers and the contributor counterpart also consider the represtened users
make difference between remove_textanswers_that_the_user_must_not_see and can_textanswer_be_seen_by clearer for the future
when users cannot or should not click specific textanswer buttons (because it does not make sense) they should still be there for consistency but should be disabled
Co-authored-by: Yannik <[email protected]>
implemented smaller changes that were quested in PR. - removed comments - itereated over enums directly - changed the name in this itereation from con_view to contributor_view (same for gen_view) - correclty formatted HTML comment - made things that should be in one line to be in one line - implemented variables for reused expected answers (on method and instance level) in test_views.py Co-authored-by: Yannik <[email protected]>
showing a tooltip with title is not possible since the tag has the disabled class and therefore does not accept any mouse events. Also: formatted code. Co-authored-by: Yannik <[email protected]>
fix #1786