Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add num_annotations to js-config group context #8939

Merged
merged 1 commit into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions h/search/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
AuthorityFilter,
DeletedFilter,
Limiter,
SharedAnnotationsFilter,
TagsAggregation,
TopLevelAnnotationsFilter,
UserFilter,
Expand All @@ -22,6 +23,7 @@
"UsersAggregation",
"get_client",
"init",
"SharedAnnotationsFilter",
)


Expand Down
14 changes: 14 additions & 0 deletions h/search/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,20 @@ def __call__(self, search, params):
)


class SharedAnnotationsFilter:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always caught by surprise about how this API works, with these callable classes to represent different filters.

🤷 Anyway this fits in the overall current design.

"""
A filter that selects only "shared" annotations and replies.

Only annotations and replies that have been shared
(`Annotation.shared=True`) will pass through this filter. "Only Me"
annotations (`Annotation.shared=False`) will be filtered out even if they
belong to the authenticated user.
"""

def __call__(self, search, params):
return search.filter("term", shared=True)


class GroupFilter:
"""
Filter that limits which groups annotations are returned from.
Expand Down
19 changes: 19 additions & 0 deletions h/services/annotation_stats.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
DeletedFilter,
Limiter,
Search,
SharedAnnotationsFilter,
TopLevelAnnotationsFilter,
UserFilter,
)
Expand Down Expand Up @@ -48,6 +49,24 @@ def group_annotation_count(self, pubid):
params = MultiDict({"limit": 0, "group": pubid})
return self._search(params)

def total_group_annotation_count(self, pubid, unshared=True):
"""
Return the count of all annotations for a group.

This counts all of the group's annotations and replies from all users.

If `unshared=True` then "Only Me" annotations and replies in the group
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had trouble passing this parameter meaning.

It might be clearer with a more verbose name? include_only_me? Not sure

(`Annotation.shared=False`) from *all* users (not just the
authenticated user) will be counted.

If `unshared=False` then no unshared annotations or replies will be
counted, not even ones from the authenticated user.
"""
search = Search(self.request)
if not unshared:
search.append_modifier(SharedAnnotationsFilter())
return search.run(MultiDict({"limit": 0, "group": pubid})).total

def _search(self, params):
search = Search(self.request)
search.append_modifier(TopLevelAnnotationsFilter())
Expand Down
4 changes: 4 additions & 0 deletions h/views/groups.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class GroupCreateEditController:
def __init__(self, context, request):
self.context = context
self.request = request
self.annotation_stats_service = request.find_service(name="annotation_stats")

@view_config(route_name="group_create", request_method="GET")
def create(self):
Expand Down Expand Up @@ -58,6 +59,9 @@ def _js_config(self):
"link": self.request.route_url(
"group_read", pubid=group.pubid, slug=group.slug
),
"num_annotations": self.annotation_stats_service.total_group_annotation_count(
group.pubid, unshared=False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is unshared always going to be False? Or are you thinking ahead of an immediate need for other type of group?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is unshared always going to be False? Or are you thinking ahead of an immediate need for other type of group?

),
}
js_config["api"]["updateGroup"] = {
"method": "PATCH",
Expand Down
24 changes: 24 additions & 0 deletions tests/unit/h/search/query_test.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# pylint:disable=too-many-lines
import datetime

import elasticsearch_dsl
Expand Down Expand Up @@ -345,6 +346,29 @@ def search(self, search, pyramid_request):
return search


class TestSharedAnnotationsFilter:
def test_it(self, pyramid_config, search, Annotation):
userid = "acct:test_user@example.com"
pyramid_config.testing_securitypolicy(userid)
# Create two shared annotations, these should be counted.
shared_annotations = [Annotation(shared=True), Annotation(shared=True)]
# Create some "Only Me" annotations, these should not be counted even
# if they belong to the authenticated user.
Annotation(shared=False)
Annotation(shared=False, userid=userid)

result = search.run(webob.multidict.MultiDict({}))

assert sorted(result.annotation_ids) == sorted(
[annotation.id for annotation in shared_annotations]
)

@pytest.fixture
def search(self, search):
search.append_modifier(query.SharedAnnotationsFilter())
return search


class TestGroupFilter:
def test_matches_only_annotations_from_specified_groups(
self, search, Annotation, groups, group_service, pyramid_request
Expand Down
30 changes: 30 additions & 0 deletions tests/unit/h/services/annotation_stats_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,27 @@ def test_group_annotation_count(
)
assert num_annotations == Search.return_value.run.return_value.total

@pytest.mark.parametrize("unshared", [True, False])
def test_total_group_annotation_count(
self, pyramid_request, svc, Search, SharedAnnotationsFilter, unshared
):
num_annotations = svc.total_group_annotation_count(
sentinel.pubid, unshared=unshared
)

Search.assert_called_once_with(pyramid_request)
if unshared:
Search.return_value.append_modifier.assert_not_called()
else:
SharedAnnotationsFilter.assert_called_once_with()
Search.return_value.append_modifier.assert_called_once_with(
SharedAnnotationsFilter.return_value
)
Search.return_value.run.assert_called_once_with(
{"limit": 0, "group": sentinel.pubid}
)
assert num_annotations == Search.return_value.run.return_value.total

@pytest.fixture
def svc(self, pyramid_request):
return AnnotationStatsService(request=pyramid_request)
Expand Down Expand Up @@ -98,6 +119,15 @@ def Search(mocker):
)


@pytest.fixture(autouse=True)
def SharedAnnotationsFilter(mocker):
return mocker.patch(
"h.services.annotation_stats.SharedAnnotationsFilter",
autospec=True,
spec_set=True,
)


@pytest.fixture(autouse=True)
def TopLevelAnnotationsFilter(mocker):
return mocker.patch(
Expand Down
9 changes: 8 additions & 1 deletion tests/unit/h/views/groups_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from h.views import groups as views


@pytest.mark.usefixtures("annotation_stats_service")
class TestGroupCreateEditController:
def test_create(self, pyramid_request, assets_env, mocker):
mocker.spy(views, "get_csrf_token")
Expand Down Expand Up @@ -38,7 +39,9 @@ def test_create(self, pyramid_request, assets_env, mocker):
}

@pytest.mark.usefixtures("routes")
def test_edit(self, factories, pyramid_request, assets_env, mocker):
def test_edit(
self, factories, pyramid_request, assets_env, mocker, annotation_stats_service
):
mocker.spy(views, "get_csrf_token")
group = factories.Group()
context = GroupContext(group)
Expand All @@ -50,6 +53,9 @@ def test_edit(self, factories, pyramid_request, assets_env, mocker):
views.get_csrf_token.assert_called_once_with( # pylint:disable=no-member
pyramid_request
)
annotation_stats_service.total_group_annotation_count.assert_called_once_with(
group.pubid, unshared=False
)
assert result == {
"page_title": "Edit group",
"js_config": {
Expand Down Expand Up @@ -79,6 +85,7 @@ def test_edit(self, factories, pyramid_request, assets_env, mocker):
"link": pyramid_request.route_url(
"group_read", pubid=group.pubid, slug=group.slug
),
"num_annotations": annotation_stats_service.total_group_annotation_count.return_value,
}
},
},
Expand Down
Loading