Show hidden annotations to group moderators #9485

marcospri · 2025-04-17T09:46:41Z

This PR modifies the default annotation filters in /search to return hidden (moderated) annotations to group moderators.

This alters the current behaviour where those annotations are only visible to their authors.

In a previous step we have refactor these to extract NIPSA-ed annotation filter to its own class:

#9574

marcospri · 2025-04-17T09:50:22Z

h/search/query.py

-    read or which are explicitly excluded by the search query.
-    """
-
+class GroupAndModerationFilter:


The current pattern of a stateful list of modifiers that are independent of each other looks fine in for very simple cases but it becomes tricky when you need to combine them.

We already have some of this classes that are incompatible (or contradictory) with each other.

So while I think combining these two in one class goes against the current style of this code I reckon it moves in the right direction.

GroupFilter and Hidden filter are always used together.

The resulting __call__() method below is pretty long and complicated, I think maybe because it's handling several separate cases in one method: when no user is logged in, when the user is logged in but there are no matching groups, and when the user is logged in and there are matching groups. Maybe it just needs to delegate to helper methods for each case?

I had another go at this after I understood it a bit better.

I reckon this is better now but let me know.

No groups

This means the current user can't read any groups (any groups in general or any of the requested groups).

We explicitly filter groups=[] to basically don't return any annotations.

This is the same behaviour as before but made explicit here for an early return.

If we just let this go to the for loop it wouldn't add any group filter.

No user

We just filter by group because we don't need to check by role in the group or authorship of the annos, just list the readable annos.

Moderator in a group

Return all annos in the group. Don't filter out hidden ones.

Nipsaed ones are now handled in a different filter class.

Not a moderator

Hidde moderated annos but include the ones authored by the user.

marcospri · 2025-04-17T09:50:47Z

tests/unit/h/search/query_test.py

@@ -2,7 +2,7 @@

 import elasticsearch_dsl
 import pytest
-import webob
+from webob.multidict import MultiDict


Couldn't resist to import this

marcospri · 2025-05-07T10:33:13Z

h/search/query.py

+from h.security.identity import Identity
+from h.security.permissions import Permission
+from h.security.permits import identity_permits


There's a circular import issue while importing these directly from identity.

marcospri · 2025-05-07T10:33:34Z

h/security/permits.py

@@ -17,6 +16,8 @@ def identity_permits(
    :param context: Context object representing the objects acted upon
    :param permission: Permission requested
    """
+    from h.security.permission_map import PERMISSION_MAP


Breaking circular import in other modules here.

marcospri · 2025-05-07T10:33:53Z

h/services/group.py

@@ -76,28 +77,26 @@ def filter_by_name(self, name=None):
            .order_by(Group.created.desc())
        )

-    def groupids_readable_by(self, user, group_ids=None):
+    def groups_readable_by(self, user: User | None, group_ids=None) -> list[Group]:


Return the full group objects instead of just the pubids.

marcospri · 2025-05-07T10:37:15Z

tests/unit/h/search/query_test.py

+        user,
+        group_service,
+        identity_permits,
+        make_annotation,


I don't particularly like this type of fixture.

This is just a function that is itself parameterized based on on other fixtures.

So whether or not you use is_nipsaed and is_hidden that function is going to be called 4 times.

I'd much prefer to just have regular parameters in that function that I can chose to call with fixture values if I need to.

I prefer to have the rest of the code reviewed and then focus on a test refactor if needed.

seanh

Posted a few questions about the search query filtering stuff and whether each of the different cases is handled correctly? I think it might be good to try and split the different cases into separate methods.

I can see why the group and hidden filters need to be combined: for each group we need to know whether the user is a moderator of that group in order to know whether to filter out hidden annotations. Makes sense 👍

I think there might be an opportunity to split out a separate NIPSAFilter, just to simplify GroupAndHiddenFilter a bit by not having to deal with NIPSA? I think NIPSA'd annotations are always hidden in all cases, if I'm not mistaken.

seanh · 2025-05-13T10:32:58Z

h/search/core.py

@@ -50,9 +50,8 @@ def __init__(
            query.Limiter(),
            query.DeletedFilter(),
            query.AuthFilter(request),
-            query.GroupFilter(request),
+            query.GroupAndModerationFilter(request),


Ok, so GroupFilter and HiddenFilter are replaced with a single GroupAndModerationFilter

seanh · 2025-05-13T10:38:34Z

h/search/query.py

+        not_nipsa = ~Q("term", nipsa=True)
+        not_hidden = ~Q("term", hidden=True)


Ok, so GroupAndHiddenFilter handles nipsa=True and hidden=True annotations, as well as groups 👍

Is not_nipsa always included in the returned result? Could it be moved to a separate NIPSAFilter that just returns search.filter(~Q("term", nipsa=True))? That might simplify this method by not having to worry about NIPSA.

I did NIPSAFilter in a separate PR 👍

h/search/query.py

seanh · 2025-05-13T10:39:56Z

h/search/query.py

+            # If there not a logged in user
+            if groups:
+                # Apply the group filter as it is
+                search = search.filter("terms", group=[g.pubid for g in groups])


So here groups would be the result of groups_readably_by(None, group_ids), so the groups that a not-logged-in user can read.

h/search/query.py

seanh · 2025-05-13T10:43:24Z

h/search/query.py

-    read or which are explicitly excluded by the search query.
-    """
-
+class GroupAndModerationFilter:


The resulting __call__() method below is pretty long and complicated, I think maybe because it's handling several separate cases in one method: when no user is logged in, when the user is logged in but there are no matching groups, and when the user is logged in and there are matching groups. Maybe it just needs to delegate to helper methods for each case?

h/search/query.py

marcospri · 2025-06-09T07:40:03Z

h/search/query.py

+            return search.filter("terms", group=[])
+
+        if not self.user:
+            return search.filter(


No logged in user, show all visible annos in the requested (& visible groups).

seanh · 2025-06-17T15:24:14Z

Rebased..

Include testing whether or not moderators can see hidden annotations.

marcospri commented Apr 17, 2025

View reviewed changes

marcospri force-pushed the show-moderated-annos branch from e569e19 to 4be16bb Compare May 7, 2025 10:32

marcospri commented May 7, 2025

View reviewed changes

marcospri requested a review from seanh May 7, 2025 10:38

seanh reviewed May 13, 2025

View reviewed changes

marcospri force-pushed the show-moderated-annos branch from 4be16bb to 6149e3a Compare May 19, 2025 12:26

marcospri changed the base branch from main to refactor-nipsa-filter May 19, 2025 12:26

marcospri force-pushed the show-moderated-annos branch 2 times, most recently from 1a2aacc to 1966a84 Compare May 19, 2025 14:06

marcospri force-pushed the refactor-nipsa-filter branch 4 times, most recently from 16aa7c5 to 6db6bdb Compare May 22, 2025 10:47

Base automatically changed from refactor-nipsa-filter to main May 22, 2025 10:56

marcospri force-pushed the show-moderated-annos branch 2 times, most recently from cab33d1 to d4dbf37 Compare May 28, 2025 13:46

marcospri self-assigned this May 30, 2025

marcospri force-pushed the show-moderated-annos branch from d4dbf37 to 05d0ee7 Compare June 9, 2025 07:33

marcospri commented Jun 9, 2025

View reviewed changes

marcospri marked this pull request as ready for review June 9, 2025 07:41

marcospri requested a review from seanh June 9, 2025 07:41

seanh force-pushed the show-moderated-annos branch from 05d0ee7 to 4c5233f Compare June 17, 2025 15:24

seanh force-pushed the show-moderated-annos branch from 4c5233f to 4450ae1 Compare June 19, 2025 17:32

seanh marked this pull request as draft June 19, 2025 17:32

seanh self-assigned this Jun 19, 2025

seanh unassigned marcospri Jun 19, 2025

seanh mentioned this pull request Jun 24, 2025

Improve the functests for the search API #9659

Open

marcospri added 2 commits June 24, 2025 18:10

Cover basic group filtering in /search on the functional tests

e764d3d

Include testing whether or not moderators can see hidden annotations.

Show hidden annotations to group moderators

44a4ea6

seanh force-pushed the show-moderated-annos branch from 4450ae1 to 44a4ea6 Compare June 24, 2025 17:10

		not_nipsa = ~Q("term", nipsa=True)
		not_hidden = ~Q("term", hidden=True)

Show hidden annotations to group moderators #9485

Are you sure you want to change the base?

Show hidden annotations to group moderators #9485

Uh oh!

Conversation

marcospri commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcospri May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

No groups

No user

Moderator in a group

Not a moderator

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanh commented Jun 17, 2025

Uh oh!

Uh oh!

marcospri commented Apr 17, 2025 •

edited

Loading

marcospri May 19, 2025 •

edited

Loading