From 47c36791f92b602411a89142d39b587ffd728712 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Tue, 15 Sep 2026 10:59:38 -0400 Subject: [PATCH] Exclude self-reviews from the contributor dashboard --- pontoon/base/forms.py | 1 - pontoon/base/get_entities.py | 5 +- pontoon/base/tests/test_get_entities.py | 110 +++++++++++++ pontoon/base/views.py | 1 - .../templates/contributors/profile.html | 4 +- pontoon/contributors/tests/test_utils.py | 148 ++++++++++++++++-- pontoon/contributors/utils.py | 18 ++- translate/src/api/entity.ts | 1 - translate/src/context/Location.tsx | 4 - .../search/components/SearchBox.test.jsx | 4 - .../modules/search/components/SearchBox.tsx | 1 - 11 files changed, 261 insertions(+), 36 deletions(-) create mode 100644 pontoon/base/tests/test_get_entities.py diff --git a/pontoon/base/forms.py b/pontoon/base/forms.py index 16ede04ec1..8525b85fc6 100644 --- a/pontoon/base/forms.py +++ b/pontoon/base/forms.py @@ -360,7 +360,6 @@ class GetEntitiesForm(forms.Form): author = forms.CharField(required=False) review_time = forms.CharField(required=False) reviewer = forms.CharField(required=False) - exclude_self_reviewed = forms.BooleanField(required=False) search = forms.CharField(required=False) entity_ids = forms.CharField(required=False) pk_only = forms.BooleanField(required=False) diff --git a/pontoon/base/get_entities.py b/pontoon/base/get_entities.py index e3f48cef39..26dea8230e 100644 --- a/pontoon/base/get_entities.py +++ b/pontoon/base/get_entities.py @@ -32,7 +32,6 @@ def get_entities_for_project_locale( author: str | None = None, review_time: str | None = None, reviewer: str | None = None, - exclude_self_reviewed: bool = False, ) -> QuerySet[Entity]: """Get project entities with locale translations.""" @@ -46,7 +45,6 @@ def get_entities_for_project_locale( review_time, author, reviewer, - exclude_self_reviewed, ) ) if pre_filter: @@ -172,7 +170,6 @@ def _time_and_user_filters( review_time: str | None, author: str | None, reviewer: str | None, - exclude_self_reviewed: bool, ) -> Iterator[Q]: if time and match("^[0-9]{12}-[0-9]{12}$", time): range = _parse_time_interval(time) @@ -207,7 +204,7 @@ def _time_and_user_filters( | Q(translation__rejected_user__email__in=emails) ) - if exclude_self_reviewed: + if reviewer or review_time: yield ~Q( Q(translation__approved_user=F("translation__user")) | Q(translation__rejected_user=F("translation__user")) diff --git a/pontoon/base/tests/test_get_entities.py b/pontoon/base/tests/test_get_entities.py new file mode 100644 index 0000000000..3458d547de --- /dev/null +++ b/pontoon/base/tests/test_get_entities.py @@ -0,0 +1,110 @@ +import pytest + +from django.utils import timezone + +from pontoon.base.get_entities import get_entities_for_project_locale +from pontoon.base.models import TranslatedResource +from pontoon.test.factories import ( + EntityFactory, + ProjectLocaleFactory, + TranslationFactory, +) + + +def _time_interval(date): + stamp = date.strftime("%Y%m%d%H%M") + return f"{stamp}-{stamp}" + + +@pytest.fixture +def reviewed_entities(resource_a, locale_a, user_a, user_b): + """ + Return two entities reviewed at the same time: one where user_a reviewed + their own translation, one where user_a reviewed user_b's translation. + """ + ProjectLocaleFactory.create(project=resource_a.project, locale=locale_a) + TranslatedResource.objects.create(resource=resource_a, locale=locale_a) + + now = timezone.now() + entities = {} + + for key, author in (("self", user_a), ("peer", user_b)): + entity = EntityFactory.create(resource=resource_a, string=f"{key} string") + TranslationFactory.create( + entity=entity, + locale=locale_a, + user=author, + approved=True, + approved_user=user_a, + approved_date=now, + date=now, + ) + entities[key] = entity + + return entities, now + + +@pytest.mark.django_db +def test_reviewer_filter_excludes_self_reviews( + reviewed_entities, resource_a, locale_a, user_a +): + """Approving your own translation is not a review performed.""" + entities, now = reviewed_entities + + matches = get_entities_for_project_locale( + user_a, + resource_a.project, + locale_a, + reviewer=user_a.email, + review_time=_time_interval(now), + ) + + assert list(matches) == [entities["peer"]] + + +@pytest.mark.django_db +def test_author_review_time_filter_excludes_self_reviews( + reviewed_entities, resource_a, locale_a, user_a, user_b +): + """Approving your own translation is not a review received.""" + entities, now = reviewed_entities + + # user_a authored the self-reviewed translation, so they received no review + assert not list( + get_entities_for_project_locale( + user_a, + resource_a.project, + locale_a, + author=user_a.email, + review_time=_time_interval(now), + ) + ) + + # user_b's translation was reviewed by user_a + assert list( + get_entities_for_project_locale( + user_a, + resource_a.project, + locale_a, + author=user_b.email, + review_time=_time_interval(now), + ) + ) == [entities["peer"]] + + +@pytest.mark.django_db +def test_self_reviewed_strings_shown_without_review_filters( + reviewed_entities, resource_a, locale_a, user_a +): + """The exclusion is scoped to the review filters, and doesn't leak elsewhere.""" + entities, now = reviewed_entities + + matches = get_entities_for_project_locale( + user_a, + resource_a.project, + locale_a, + author=user_a.email, + time=_time_interval(now), + ) + + assert list(matches) == [entities["self"]] diff --git a/pontoon/base/views.py b/pontoon/base/views.py index 77772cd2f6..000e174761 100755 --- a/pontoon/base/views.py +++ b/pontoon/base/views.py @@ -378,7 +378,6 @@ def entities(request: HttpRequest): "author", "review_time", "reviewer", - "exclude_self_reviewed", "tag", ) form_data = { diff --git a/pontoon/contributors/templates/contributors/profile.html b/pontoon/contributors/templates/contributors/profile.html index 06546aa7f8..5fc0e655d3 100644 --- a/pontoon/contributors/templates/contributors/profile.html +++ b/pontoon/contributors/templates/contributors/profile.html @@ -413,7 +413,9 @@

{{ contribution_graph.title }}

>
  • - Reviews performed
    • diff --git a/pontoon/contributors/tests/test_utils.py b/pontoon/contributors/tests/test_utils.py index 8a58e5a316..1f6f324009 100644 --- a/pontoon/contributors/tests/test_utils.py +++ b/pontoon/contributors/tests/test_utils.py @@ -65,7 +65,32 @@ def action_c(translation_a): @pytest.fixture -def action_user_a(translation_a, user_a): +def peer_translation(locale_a, project_locale_a, entity_a, user_b): + """Return a translation by another user so reviews of it are peer reviews.""" + return TranslationFactory( + entity=entity_a, + locale=locale_a, + user=user_b, + string="Translation by user_b", + value=["Translation by user_b"], + ) + + +@pytest.fixture +def action_user_a(peer_translation, user_a): + action = ActionLog.objects.create( + action_type=ActionLog.ActionType.TRANSLATION_APPROVED, + performed_by=user_a, + translation=peer_translation, + ) + action.created_at = timezone.now() - relativedelta(months=1) + action.save() + return action + + +@pytest.fixture +def self_approval_user_a(translation_a, user_a): + """Return user_a approving their own translation.""" action = ActionLog.objects.create( action_type=ActionLog.ActionType.TRANSLATION_APPROVED, performed_by=user_a, @@ -89,12 +114,12 @@ def action_user_b(translation_a, user_b): @pytest.fixture -def yesterdays_action_user_a(translation_a, user_a): +def yesterdays_action_user_a(peer_translation, user_a): current_date = timezone.now() action = ActionLog.objects.create( action_type=ActionLog.ActionType.TRANSLATION_APPROVED, performed_by=user_a, - translation=translation_a, + translation=peer_translation, ) if current_date.day == 1: # First day of the month, so we instead set created_at to be earlier today @@ -198,7 +223,9 @@ def test_get_approvals_charts_data_without_actions(user_a): @pytest.mark.django_db -def test_get_approvals_charts_data_with_actions(user_a, action_user_a, action_user_b): +def test_get_approvals_charts_data_with_actions( + user_a, self_approval_user_a, action_user_b +): data = utils.get_approvals_charts_data(user_a) assert data["approval_rates"] == [0] * 11 + [100] @@ -270,7 +297,9 @@ def test_get_contributions_map_without_actions(user_a, user_b): @pytest.mark.django_db -def test_get_contributions_map_with_actions(user_a, action_user_a, user_b): +def test_get_contributions_map_with_actions( + user_a, action_user_a, action_user_b, user_b +): map = utils.get_contributions_map(user_a, user_b) for key, value in map.items(): @@ -280,6 +309,103 @@ def test_get_contributions_map_with_actions(user_a, action_user_a, user_b): assert value.exists() +@pytest.mark.django_db +def test_get_contributions_map_excludes_self_reviews(user_a, user_b, translation_a): + """Self-reviews count as neither performed nor received reviews.""" + ActionLog.objects.create( + action_type=ActionLog.ActionType.TRANSLATION_APPROVED, + performed_by=user_a, + translation=translation_a, + ) + + map = utils.get_contributions_map(user_a, user_b) + + assert not map["user_reviews"].exists() + assert not map["peer_reviews"].exists() + assert not map["all_user_contributions"].exists() + assert not map["all_contributions"].exists() + + +@pytest.mark.django_db +def test_get_contributions_map_keeps_reviews_of_imported_translations( + user_a, user_b, locale_a, project_locale_a, entity_a +): + """A translation without an author is nobody's own work, so reviewing it counts.""" + imported = TranslationFactory( + entity=entity_a, + locale=locale_a, + user=None, + string="Imported translation", + value=["Imported translation"], + ) + ActionLog.objects.create( + action_type=ActionLog.ActionType.TRANSLATION_REJECTED, + performed_by=user_a, + translation=imported, + ) + + map = utils.get_contributions_map(user_a, user_b) + + assert map["user_reviews"].exists() + assert not map["peer_reviews"].exists() + + +@pytest.mark.django_db +def test_get_contributions_map_excludes_obsolete_entities( + user_a, user_b, locale_a, project_locale_a, resource_a +): + """The translate view can't show obsolete strings, so they aren't counted.""" + obsolete_entity = EntityFactory.create( + resource=resource_a, string="Obsolete string", obsolete=True + ) + translation = TranslationFactory( + entity=obsolete_entity, + locale=locale_a, + user=user_b, + string="Translation of an obsolete string", + value=["Translation of an obsolete string"], + ) + ActionLog.objects.create( + action_type=ActionLog.ActionType.TRANSLATION_APPROVED, + performed_by=user_a, + translation=translation, + ) + + map = utils.get_contributions_map(user_a, user_b) + + assert not map["user_reviews"].exists() + assert not map["all_contributions"].exists() + + +@pytest.mark.django_db +def test_get_contributions_map_excludes_disabled_projects(user_a, user_b, locale_a): + """The translate view can't show strings of disabled projects either.""" + project = ProjectFactory.create( + slug="disabled_project", name="Disabled Project", disabled=True + ) + resource = ResourceFactory.create( + project=project, path="resource_disabled.po", format="gettext" + ) + entity = EntityFactory.create(resource=resource, string="Disabled string") + translation = TranslationFactory( + entity=entity, + locale=locale_a, + user=user_b, + string="Translation in a disabled project", + value=["Translation in a disabled project"], + ) + ActionLog.objects.create( + action_type=ActionLog.ActionType.TRANSLATION_APPROVED, + performed_by=user_a, + translation=translation, + ) + + map = utils.get_contributions_map(user_a, user_b) + + assert not map["user_reviews"].exists() + assert not map["all_contributions"].exists() + + @pytest.mark.django_db def test_get_contribution_graph_data_without_actions(user_a, user_b): assert utils.get_contribution_graph_data(user_a, user_b) == ( @@ -301,12 +427,12 @@ def test_get_contribution_graph_data_with_actions(user_a, action_user_a, user_b) @pytest.mark.django_db -def test_get_contribution_graph_data_for_year(user_a, user_b, translation_a): +def test_get_contribution_graph_data_for_year(user_a, user_b, peer_translation): # Action in 2025 action_2025 = ActionLog.objects.create( action_type=ActionLog.ActionType.TRANSLATION_APPROVED, performed_by=user_a, - translation=translation_a, + translation=peer_translation, ) action_2025.created_at = timezone.make_aware(datetime(2025, 6, 15)) action_2025.save() @@ -315,7 +441,7 @@ def test_get_contribution_graph_data_for_year(user_a, user_b, translation_a): action_2026 = ActionLog.objects.create( action_type=ActionLog.ActionType.TRANSLATION_APPROVED, performed_by=user_a, - translation=translation_a, + translation=peer_translation, ) action_2026.created_at = timezone.make_aware(datetime(2026, 1, 1)) action_2026.save() @@ -385,13 +511,13 @@ def test_get_contribution_timeline_data_with_actions( @pytest.mark.django_db -def test_get_contribution_timeline_data_for_year(user_a, user_b, translation_a): +def test_get_contribution_timeline_data_for_year(user_a, user_b, peer_translation): # Reviews in two different months of 2025 for review_date in [datetime(2025, 6, 15), datetime(2025, 12, 10)]: action = ActionLog.objects.create( action_type=ActionLog.ActionType.TRANSLATION_APPROVED, performed_by=user_a, - translation=translation_a, + translation=peer_translation, ) action.created_at = timezone.make_aware(review_date) action.save() @@ -400,7 +526,7 @@ def test_get_contribution_timeline_data_for_year(user_a, user_b, translation_a): action_2026 = ActionLog.objects.create( action_type=ActionLog.ActionType.TRANSLATION_APPROVED, performed_by=user_a, - translation=translation_a, + translation=peer_translation, ) action_2026.created_at = timezone.make_aware(datetime(2026, 6, 15)) action_2026.save() diff --git a/pontoon/contributors/utils.py b/pontoon/contributors/utils.py index 08494769e0..3e950343e5 100644 --- a/pontoon/contributors/utils.py +++ b/pontoon/contributors/utils.py @@ -306,7 +306,10 @@ def get_contributions_map( """ actions_base = cast(ActionLogQuerySet, ActionLog.objects) - actions = actions_base.visible_for(viewer) + actions = actions_base.visible_for(viewer).filter( + translation__entity__obsolete=False, + translation__entity__resource__project__disabled=False, + ) if contribution_period is not None: actions = actions.filter(contribution_period, is_implicit_action=False) @@ -316,15 +319,15 @@ def get_contributions_map( ActionLog.ActionType.TRANSLATION_REJECTED, ] + non_self_reviews = actions.filter(action_type__in=review_action_types).exclude( + performed_by=F("translation__user") + ) + user_translations = actions.filter( performed_by=contributor, action_type=ActionLog.ActionType.TRANSLATION_CREATED ) - user_reviews = actions.filter( - performed_by=contributor, action_type__in=review_action_types - ) - peer_reviews = actions.filter( - translation__user=contributor, action_type__in=review_action_types - ) + user_reviews = non_self_reviews.filter(performed_by=contributor) + peer_reviews = non_self_reviews.filter(translation__user=contributor) all_user_contributions = user_translations | user_reviews @@ -541,7 +544,6 @@ def get_contribution_timeline_data( url_params = { "author": contributor.email, "review_time": time_str, - "exclude_self_reviewed": "", } title += f" in {intcomma(p_count)} project{pluralize(p_count)}" diff --git a/translate/src/api/entity.ts b/translate/src/api/entity.ts index 1d5a8f7441..6261f47874 100644 --- a/translate/src/api/entity.ts +++ b/translate/src/api/entity.ts @@ -164,7 +164,6 @@ function buildFetchPayload( 'created_time', 'reviewer', 'review_time', - 'exclude_self_reviewed', ] as const) { const value = location[key]; if (value) { diff --git a/translate/src/context/Location.tsx b/translate/src/context/Location.tsx index 2d6a86aead..fb8fba9e00 100644 --- a/translate/src/context/Location.tsx +++ b/translate/src/context/Location.tsx @@ -34,7 +34,6 @@ export type Location = { created_time: string | null; reviewer: string | null; review_time: string | null; - exclude_self_reviewed: boolean; }; export const emptyParams = { @@ -53,7 +52,6 @@ export const emptyParams = { created_time: null, reviewer: null, review_time: null, - exclude_self_reviewed: false, }; export const Location = createContext({ @@ -140,7 +138,6 @@ function parse( created_time: params.get('created_time'), reviewer: params.get('reviewer'), review_time: params.get('review_time'), - exclude_self_reviewed: params.has('exclude_self_reviewed'), list: null, }; return location; @@ -179,7 +176,6 @@ function stringify(prev: Location, next: string | Partial) { 'created_time', 'reviewer', 'review_time', - 'exclude_self_reviewed', ] as const) { const value = key in next ? next[key] : prev[key]; if (value) { diff --git a/translate/src/modules/search/components/SearchBox.test.jsx b/translate/src/modules/search/components/SearchBox.test.jsx index 310bf1a538..0e4525d81a 100644 --- a/translate/src/modules/search/components/SearchBox.test.jsx +++ b/translate/src/modules/search/components/SearchBox.test.jsx @@ -199,7 +199,6 @@ describe('', () => { created_time: null, reviewer: null, review_time: null, - exclude_self_reviewed: false, entity: 0, }); }); @@ -216,7 +215,6 @@ describe('', () => { created_time: '202606120818-202606120818', reviewer: 'user@example.com', review_time: '202606120818-202606120818', - exclude_self_reviewed: true, }} project={PROJECT} searchAndFilters={SEARCH_AND_FILTERS} @@ -235,7 +233,6 @@ describe('', () => { expect(pushed.created_time).toBeNull(); expect(pushed.reviewer).toBeNull(); expect(pushed.review_time).toBeNull(); - expect(pushed.exclude_self_reviewed).toBe(false); }); it('sets correct status', () => { @@ -281,7 +278,6 @@ describe('', () => { created_time: null, reviewer: null, review_time: null, - exclude_self_reviewed: false, entity: 0, list: null, }); diff --git a/translate/src/modules/search/components/SearchBox.tsx b/translate/src/modules/search/components/SearchBox.tsx index 980de14225..2d4eceab89 100644 --- a/translate/src/modules/search/components/SearchBox.tsx +++ b/translate/src/modules/search/components/SearchBox.tsx @@ -308,7 +308,6 @@ export function SearchBoxBase({ created_time: null, reviewer: null, review_time: null, - exclude_self_reviewed: false, entity: 0, // With the new results, the current entity might not be available anymore. list: parameters.list ?? null, ...getSearchUpdates(searchOptions),