Customizable evaluation results filters #2232

jooooosef · 2024-06-24T20:49:04Z

fix #1786

evap/results/templates/results_evaluation_detail.html

janno42

looks good, most parts work as expected
tests not reviewed yet

evap/results/templates/results_evaluation_detail.html

evap/results/views.py

evap/results/templates/results_evaluation_detail.html

evap/results/views.py

evap/results/tests/test_views.py

richardebeling · 2024-08-05T18:12:47Z

evap/results/tests/test_views.py

@@ -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):


Why was this removed?

that was an accident I added it again

evap/results/tests/test_views.py

…py müssen wir uns nochmal anschauen, da failed noch ein test

…est_tools zusammengeführt, jetzt alles clean

…aber jetzt geht wieder

…tor and responsible test classes

…swers

…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

removed unnecessary comment and fixed HTML broken indentations and whitespaces

richardebeling · 2024-11-25T18:39:34Z

evap/results/templates/results_evaluation_detail.html

@@ -21,7 +21,7 @@
    {% if evaluation.state != evaluation.State.PUBLISHED %}
        <div class="alert alert-warning">{% translate 'This is a preview. The results have not been published yet.' %}</div>
    {% endif %}
-
+    <!--manager is automatically a reviewer (so this are all groups that can see the buttons)-->


Suggested change



{# manager is automatically a reviewer (so this are all groups that can see the buttons) #}

so this is not part of the rendered output

richardebeling · 2024-11-25T18:46:35Z

evap/results/tests/test_views.py

+    def test_default_view(
+        self,
+    ):


Suggested change

def test_default_view(

self,

):

def test_default_view(self):

richardebeling · 2024-11-25T18:54:07Z

evap/results/tests/test_views.py

-        self.assertNotIn(".responsible_contributor_orig_notreviewed.", page)
-        self.assertIn(".responsible_contributor_additional_orig_published.", page)
-        self.assertNotIn(".responsible_contributor_additional_orig_hidden.", page)
+    def helper_test_general(self, user, view_general_results, textanswers_in):


Regarding textanswers_in: What do you think about expected_visible_textanswers? (Repeated below)

richardebeling · 2024-11-25T18:57:19Z

evap/results/tests/test_views.py

+            user = "[email protected]"
+
+            self.helper_test_general(
+                user,


You should always be able to just use self.student, self.manager, .... This allows someone reading the code to not need to keep context in memory, all lines are verifiable in isolation.

Suggested change

user = "[email protected]"

self.helper_test_general(

user,

self.helper_test_general(

self.manager,

richardebeling · 2024-11-25T19:00:50Z

evap/results/tests/test_views.py

+                ".responsible_contributor_orig_published.",
+                ".responsible_contributor_changed_published.",
+                ".responsible_contributor_orig_private.",
+                ".responsible_contributor_additional_orig_published.",


We can put this block of 4 expected answers and the block above of 3 expected answers into some well-named variable, which reduced duplication and makes the test easier to read through.

richardebeling · 2024-11-25T19:09:21Z

evap/results/tools.py

+        if view_general_results == ViewGeneralResults.RATINGS:
+            return False


I think this branch and the corresponding final else branch, can both be removed.

richardebeling · 2024-11-25T19:15:00Z

evap/results/views.py

@@ -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


Why is all this new code here necessary now, which part of this is is not covered by the access computation methods above?

richardebeling · 2024-11-25T19:19:17Z

evap/results/views.py

+            request.GET.get(
+                "view_contributor_results",
+                ViewContributorResults.FULL,
+            )


I don't think these hit the 120 columns limit, do they?

Suggested change

request.GET.get(

"view_contributor_results",

ViewContributorResults.FULL,

)

request.GET.get("view_contributor_results", ViewContributorResults.FULL)

richardebeling · 2024-11-25T19:21:21Z

evap/results/views.py

-    # 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"


Why is this no longer needed?

richardebeling · 2024-11-25T19:22:56Z

evap/staff/templates/staff_user_form.html

@@ -101,7 +101,7 @@ <h5 class="card-title me-auto">{% translate 'Export evaluation results' %}</h5>
                                    {% for evaluation in semester_evaluations.list %}
                                        <li>
                                            {% if evaluation|can_results_page_be_seen_by:form.instance %}
-                                                <a href="{% url 'results:evaluation_detail' semester_evaluations.grouper.id evaluation.id %}?view=export&contributor_id={{ form.instance.id }}">
+                                                <a href="{% url 'results:evaluation_detail' semester_evaluations.grouper.id evaluation.id %}?view_contributor_results=personal&contributor_id={{ form.instance.id }}">
                                                    {{ evaluation.full_name }}
                                                </a>


random line of code: You updated the bootstrap submodule. Was this intentional / required for anything? If not, please revert.

janno42 · 2024-11-25T17:16:45Z

evap/results/tools.py

+class ViewGeneralResults(Enum):
+    @property
+    def do_not_call_in_templates(self):
+        return False


Suggested change

return False

return True

janno42 · 2024-11-25T17:23:50Z

evap/results/tools.py

@@ -473,49 +493,54 @@ def textanswers_visible_to(contribution):
    return TextAnswerVisibility(visible_by_contribution=sorted_contributors, visible_by_delegation_count=num_delegates)


-def can_textanswer_be_seen_by(  # noqa: PLR0911
+# Filter textanswers based on user selection


Suggested change

# Filter textanswers based on user selection

probably not needed

janno42 · 2024-11-25T17:44:02Z

evap/results/tools.py

+        if view_general_results == ViewGeneralResults.RATINGS:
+            return False
+        if view_general_results == ViewGeneralResults.FULL:
+            # reviewer can see everything


Suggested change

# reviewer can see everything

janno42 · 2024-11-25T19:18:54Z

evap/results/views.py

+            if Contribution.objects.filter(
+                contributor=user,
+                evaluation=evaluation,


Suggested change

if Contribution.objects.filter(

contributor=user,

evaluation=evaluation,

if evaluation.contributions.filter(

contributor=user,

janno42 · 2024-11-25T19:55:22Z

evap/results/tests/test_views.py

+    contributor_views = [ViewContributorResults.FULL, ViewContributorResults.RATINGS, ViewContributorResults.PERSONAL]
+    general_views = [ViewGeneralResults.FULL, ViewGeneralResults.RATINGS]


you don't need these, you can iterate over the enums

janno42 · 2024-11-25T20:02:08Z

evap/results/tests/test_views.py

+        for con_view in self.contributor_views:
+            page = self.app.get(
+                f"/results/semester/1/evaluation/1?view_general_results={view_general_results.value}&view_contributor_results={con_view.value}",
+                user=user,
+            )


Suggested change

for con_view in self.contributor_views:

page = self.app.get(

f"/results/semester/1/evaluation/1?view_general_results={view_general_results.value}&view_contributor_results={con_view.value}",

user=user,

)

for contributor_view in self.contributor_views:

page = self.app.get(

f"/results/semester/1/evaluation/1?view_general_results={view_general_results.value}&view_contributor_results={contributor_view.value}",

user=user,

)

janno42 · 2024-11-25T20:04:54Z

evap/results/tests/test_views.py

+        for gen_view in self.general_views:
+            page = self.app.get(
+                f"/results/semester/1/evaluation/1?view_contributor_results={view_contributor_results.value}&view_general_results={gen_view.value}",
+                user=user,
+            )
+
+            for answer in textanswers_in:
+                self.assertIn(answer, page)
+            for answer in textanswers_not_in:
+                self.assertNotIn(answer, page)


Suggested change

for gen_view in self.general_views:

page = self.app.get(

f"/results/semester/1/evaluation/1?view_contributor_results={view_contributor_results.value}&view_general_results={gen_view.value}",

user=user,

)

for answer in textanswers_in:

self.assertIn(answer, page)

for answer in textanswers_not_in:

self.assertNotIn(answer, page)

for general_view in self.general_views:

page = self.app.get(

f"/results/semester/1/evaluation/1?view_contributor_results={view_contributor_results.value}&view_general_results={general_view.value}",

user=user,

)

for answer in textanswers_in:

self.assertIn(answer, page)

for answer in textanswers_not_in:

self.assertNotIn(answer, page)

janno42 · 2024-11-25T20:54:15Z

evap/results/templates/results_evaluation_detail.html

+                    <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>


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."

janno42 · 2024-11-25T21:20:44Z

evap/results/templates/results_evaluation_detail.html

+                <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>


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."

jooooosef marked this pull request as draft June 24, 2024 20:49

jooooosef changed the title ~~Customizable evaluation results filters #1786~~ Customizable evaluation results filters Jun 24, 2024

ybrnr force-pushed the iss1786 branch 4 times, most recently from 0127464 to ca11745 Compare July 8, 2024 15:48

janno42 reviewed Jul 15, 2024

View reviewed changes

evap/results/templates/results_evaluation_detail.html Outdated Show resolved Hide resolved

ybrnr force-pushed the iss1786 branch from 5341dc8 to 25c508d Compare July 15, 2024 20:39

janno42 requested review from richardebeling and Kakadus July 22, 2024 17:04

janno42 requested changes Jul 22, 2024

View reviewed changes

richardebeling requested changes Aug 5, 2024

View reviewed changes

ybrnr force-pushed the iss1786 branch from 3d94b85 to c509020 Compare October 14, 2024 15:52

richardebeling changed the title ~~Customizable evaluation results filters~~ Customizable evaluation results filters Oct 14, 2024

ybrnr force-pushed the iss1786 branch 2 times, most recently from 9bd5cea to 8aaee00 Compare October 21, 2024 16:22

ybrnr force-pushed the iss1786 branch 2 times, most recently from e77a156 to 73c3a65 Compare October 28, 2024 22:50

jooooosef requested review from richardebeling and janno42 November 18, 2024 22:03

jooooosef marked this pull request as ready for review November 18, 2024 22:04

ybrnr and others added 9 commits November 25, 2024 17:30

alle changes ohne tests

ffcc5ed

tests jetzt hinzugefügt mit neuer struktur (schon wieder) test_tools.…

5314120

…py müssen wir uns nochmal anschauen, da failed noch ein test

minimal_test_data_results.json geändert, ist jetzt cooler, außerdem t…

ee4f6e8

…est_tools zusammengeführt, jetzt alles clean

vscode hat komische dinge in results_evaluation_detail.html gemacht, …

de166d3

…aber jetzt geht wieder

button description

66f3907

correct redirect link when exporting as manager

fc44043

tests shortened, new test for contributor

4c9e178

added tests for delegates for contributor and responsible in contribu…

8d69b5d

…tor and responsible test classes

test

9460ceb

ybrnr and others added 24 commits November 25, 2024 17:30

added small test for contributor default view

4489254

new can_textanswer_be_seen_by structure

3da2c2c

disable general view buttons if user cannot view general textanswers

2b5dfbb

disable filter buttons, if user cannot see general/contributor textan…

03dc670

…swers

enum for differnet button filter options

026b116

fixed imports

1143d91

started to change test structure... (again)

d8ce6ff

tests weiter geändert

2f5662a

changed some tests

4ebb82c

manager test now with staff mode and removed old tests

f0d7015

removed unnecessary test

6fd71cf

made the active conitions for buttons use proper Enums

01f8efd

added helper test to see if buttons are disabled

f48e5c4

buttons cannot be active, when disabled; created next_step file to tr…

cf9bcce

…ack on what problem we are working atm

added comments in code to work on next time

eb0c654

reformatted files

2e3b187

(partially) fixed display of button groups

485f997

made the can_see_general_textanswers and the contributor counterpart also consider the represtened users

changed filter logic for general

d897490

added prints for testing

31d575f

added comment

3dc87f1

make difference between remove_textanswers_that_the_user_must_not_see and can_textanswer_be_seen_by clearer for the future

disable textanswer buttons

c81b84b

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

linter happy now :)

c9c224a

removed comments

c9ffbef

cleanup

c76acae

removed unnecessary comment and fixed HTML broken indentations and whitespaces

ybrnr force-pushed the iss1786 branch from a1809d3 to c76acae Compare November 25, 2024 16:30

removed txt file

c852a04

richardebeling reviewed Nov 25, 2024

View reviewed changes

janno42 reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customizable evaluation results filters #2232

Customizable evaluation results filters #2232

jooooosef commented Jun 24, 2024 •

edited

Loading

janno42 left a comment

richardebeling Aug 5, 2024

ybrnr Oct 14, 2024

richardebeling Nov 25, 2024

richardebeling Nov 25, 2024

richardebeling Nov 25, 2024

richardebeling Nov 25, 2024

richardebeling Nov 25, 2024

richardebeling Nov 25, 2024

richardebeling Nov 25, 2024

richardebeling Nov 25, 2024

richardebeling Nov 25, 2024

richardebeling Nov 25, 2024

janno42 Nov 25, 2024

janno42 Nov 25, 2024

janno42 Nov 25, 2024

janno42 Nov 25, 2024

janno42 Nov 25, 2024

janno42 Nov 25, 2024

janno42 Nov 25, 2024

janno42 Nov 25, 2024

janno42 Nov 25, 2024

janno42 Nov 25, 2024

	<!--manager is automatically a reviewer (so this are all groups that can see the buttons)-->
	{# manager is automatically a reviewer (so this are all groups that can see the buttons) #}

		if view_general_results == ViewGeneralResults.RATINGS:
		return False

		@@ -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

		contributor_views = [ViewContributorResults.FULL, ViewContributorResults.RATINGS, ViewContributorResults.PERSONAL]
		general_views = [ViewGeneralResults.FULL, ViewGeneralResults.RATINGS]

Customizable evaluation results filters #2232

Are you sure you want to change the base?

Customizable evaluation results filters #2232

Conversation

jooooosef commented Jun 24, 2024 • edited Loading

janno42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jooooosef commented Jun 24, 2024 •

edited

Loading