Show hidden annotations to group moderators #9485

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

Testing

TODO

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 add another go at this after I understood it a bit better.

I reckon this is a bit 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.

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

h/search/query.py

+        not_hidden = ~Q("term", hidden=True)
+
+        if not self.user:
+            # If there not a logged in user


Suggested change

# If there not a logged in user

# If they're not a logged in user.

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.

seanh · 2025-05-13T10:40:27Z

h/search/query.py

+
+        query_clauses = []
+
+        # If t he user is logged in and we are filtering by groups


Suggested change

# If t he user is logged in and we are filtering by groups

# If the user is logged in and we are filtering by groups

I'm not sure that this comment is correct. I think we could be filtering by groups (i.e. group_ids could be non-None) but none of the group IDs are readable by this user (so groups is None).

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?

seanh · 2025-05-13T10:46:28Z

h/search/query.py

+            if user_is_moderator:
+                # For moderators:
+                # - We show all their annos
+                # - We don't filter out hidden annos
+                # - We do hide NIPSA'd annos
+                query_clauses.append(group_annotations & (not_nipsa | user_annotations))


I'm not sure why this stuff for moderators isn't needed in the previous case above (when the user is logged in but groups is falsey)?

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

Show hidden annotations to group moderators

cab33d1

marcospri force-pushed the show-moderated-annos branch from 1966a84 to cab33d1 Compare May 22, 2025 13:02

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

	# If there not a logged in user
	# If they're not a logged in user.


		query_clauses = []

		# If t he user is logged in and we are filtering by groups

	# If t he user is logged in and we are filtering by groups
	# If the user is logged in and we are filtering by groups

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!

Testing

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

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!

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!

marcospri commented Apr 17, 2025 •

edited

Loading