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
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
…py müssen wir uns nochmal anschauen, da failed noch ein test
…est_tools zusammengeführt, jetzt alles clean
…aber jetzt geht wieder
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]>
Removed tooltip from individual buttons and added a big one on label of the results button groups, since it is not simply possible to show a tooltip on a disabled button. Co-authored-by: Yannik <[email protected]>
Test seems to be deleted by accident. Added it again.
should (maybe?) be a lil faster. But was recommend so here.
This uses the db to check if one of the represented users is a responsible.
|
I just realised that when I use the contributor and go in a contribution where I dont have general_textanswers rights but can edit change the contributor settings that the value for the view_general_results parameter is not set correctly. Why is that? @janno42 |
removed tooltips and changed some labels
richardebeling
requested review from
richardebeling,
niklasmohrin and
Kakadus
Probably because you don't set the value and in |
name was "general" even though it tests contributor
… for general and contributor results
changed some code to be more clear and used better naming
There was a problem hiding this comment.
I wonder if we should try to make the columns align to make it look nicer, what do you think @janno42 ?
This is the current site:
There was a problem hiding this comment.
We discussed that and both alignments have their disadvantages. I originally also thought right-aligned might look better, so yes, let's move them :)
There was a problem hiding this comment.
Ah, but do you mean to keep the width of all elements the same, but move the top row to right? I thought that we would make the right border of "general results" and "contributor results" align, and then have the two "ratings and texts" buttons and the two "ratings" buttons be vertically aligned (with no button over "only mine").
In the end, I don't really care about which version we use
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| ".responsible_contributor_additional_orig_deleted.", | ||
| ] | ||
|
|
||
| standard_general_textanswers = [ # subset of general_textanswers |
There was a problem hiding this comment.
I feel like this is intended to be "set of textanswers that are visible to everyone who can see textanswers"? If yes, I'd suggest to name/comment like that.
| def test_manager(self): | ||
| user = self.manager | ||
| self.check_with_view(user, []) | ||
| with run_in_staff_mode(self): # in staff mode, the manager can see everything except deleted or changed |
There was a problem hiding this comment.
comment doesn't match assertion below, we assert that everything from self.standard_general_textanswers is visible, which includes general_changed_published.
I would, without checking more deeply, expect that noone can ever see deleted or the originals of changed answers? In that case, I would also try to generalize that out, so that the logic here doesn't have to list any answers at all, but literally just says: "As a manager, I can see all comments that could possibly be shown to any user".
| expected_visible_textanswers, | ||
| general=ViewGeneralResults, | ||
| contributor=ViewContributorResults, | ||
| all_textanswers=general_textanswers + contributor_textanswers, |
There was a problem hiding this comment.
Quick Ctrl+F says all_textanswers is never passed?
| user_represents_general_visibilty_contributor = any( | ||
| evaluation.is_user_contributor(user) | ||
| and evaluation.contributions.filter( | ||
| contributor=user, | ||
| textanswer_visibility=Contribution.TextAnswerVisibility.GENERAL_TEXTANSWERS, | ||
| ).exists() | ||
| for user in represented_users | ||
| ) |
There was a problem hiding this comment.
Semantically correct and nice code, but: We also need to worry about performance here. is_user_contributor hits the database once, the .exists() query hits the database once, all of this happens n times for n represented users. Can we maybe express this as a single queryset instead?
(same thing applies to the other queries above as well)
fix #1786