Customizable evaluation results filters

[jooooosef]

fix #1786

[janno42]

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

[Kakadus]

some last comments from me :)

[janno42]

This nice, want have.
(After changing these little things)

[jooooosef]

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.
Example:
user: [email protected]
contribution: Operating Systems I
click on ratings

Why is that? @janno42

[janno42]

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. Example: user: [email protected] contribution: Operating Systems I click on ratings

Why is that? @janno42

Probably because you don't set the value and in evaluation_detail_parse_get_parameters the default is FULL?

[niklasmohrin]

Looks mostly good

[niklasmohrin]

The second line is indented too far here

[niklasmohrin]

Suggested change

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

[niklasmohrin]

Do you mean this?

Suggested change

	page_with_ratings_general_get_parameter = self.app.get(
	self.url + "?view_contributor_results=ratings", user=self.manager
	)
	page_with_ratings_contributor_get_parameter = self.app.get(
	self.url + "?view_contributor_results=ratings", user=self.manager
	)

[niklasmohrin]

I think you can refactor this to merge the two helper_test_* methods:

from itertools import product

def check_with_view(
    self,
    user,
    all_textanswers,
    expected_visible_textanswers,
    general=ViewGeneralResults,
    contributor=ViewContributorResults,
):
    textanswers_not_in = list(set(all_textanswers) - set(expected_visible_textanswers))
    for general_view, contributor_view in product(general, contributor):
        page = self.app.get(
            f"/results/semester/1/evaluation/1?view_general_results={general_view.value}&view_contributor_results={contributor_view.value}",
            user=user,
        )

        for answer in expected_visible_textanswers:
            self.assertIn(answer, page)
        for answer in textanswers_not_in:
            self.assertNotIn(answer, page)

The above checks all combinations of the given general and contributor views, and the arguments each default to checking all variants. You can still have helper_test_general and helper_test_contributor, but let them use check_with_view so that their logic is not duplicated. Maybe this would even be a way to shorten some parts like the following?

        self.helper_test_general(user, ViewGeneralResults.FULL, [])
        self.helper_test_general(user, ViewGeneralResults.RATINGS, [])

        self.helper_test_contributor(user, ViewContributorResults.FULL, [])
        self.helper_test_contributor(user, ViewContributorResults.RATINGS, [])
        self.helper_test_contributor(user, ViewContributorResults.PERSONAL, [])

[niklasmohrin]

In particular, the case "person sees nothing" seems to be checked a couple of times

[niklasmohrin]

You can just assign this once at the beginning of the function

[niklasmohrin]

I find this a bit hard to understand, how do you feel about the following?

Suggested change

	if view_contributor_results == ViewContributorResults.RATINGS:
	return False
	if user.is_reviewer:
	return True
	if (
	view_contributor_results == ViewContributorResults.PERSONAL or textanswer.is_private
	): # private textanswers should only be seen by the contributor and reviewer
	return contributor == user
	if view_contributor_results == ViewContributorResults.FULL:
	# users can see textanswers if the contributor is one of their represented users (which includes the user itself)
	if contributor in represented_users:
	return True
	match view_contributor_results:
	case ViewContributorResults.RATINGS:
	return False
	case ViewContributorResults.PERSONAL:
	return user.is_reviewer or contributor == user
	case ViewContributorResults.FULL:
	if user.is_reviewer:
	return True
	if textanswer.is_private:
	return user == contributor
	return contributor in represented_users

[niklasmohrin]

So just to make sure: We checked that textanswers_visible_to does not need any changes?

[niklasmohrin]

You can merge these two, and also the contributor one below:

Suggested change

	try:
	view_contributor_results = ViewContributorResults(
	request.GET.get("view_contributor_results", ViewContributorResults.FULL)
	)
	except ValueError as e:
	raise BadRequest from e
	try:
	view_general_results = ViewGeneralResults(
	request.GET.get(
	"view_general_results",
	ViewGeneralResults.FULL,
	)
	)
	except ValueError as e:
	raise BadRequest from e
	try:
	view_general_results = ViewGeneralResults(request.GET.get("view_general_results", ViewGeneralResults.FULL))
	view_contributor_results = ViewContributorResults(
	request.GET.get("view_contributor_results", ViewContributorResults.FULL)
	)
	contributor = get_object_or_404(UserProfile, pk=request.GET.get("contributor_id", request.user.id))
	except ValueError as e:
	raise BadRequest from e

[niklasmohrin]

These two lines can just be represented_users = [view_as_user, *view_as_user.represented_users.all()]

[niklasmohrin]

We can just write contributor_personal = evaluation.is_user_contributor(view_as_user) from the start

[niklasmohrin]

Same here: contributor_textanswers = view_as_user.is_reviewer or evaluation.user_is_contributor(view_as_user)

[niklasmohrin]

and for the third one:

    contributor_textanswers = view_as_user.is_reviewer or evaluation.user_is_contributor(view_as_user)
    contributor_personal = evaluation.is_user_contributor(view_as_user)

    user_represents_responsible = any(user in represented_users for user in evaluation.course.responsibles.all())
    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
    )

    general_textanswers = (
        view_as_user.is_reviewer or user_represents_responsible or user_represents_general_visibilty_contributor
    )

[niklasmohrin]

@richardebeling ?