Add num_annotations to js-config group context
#8939
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seanh
changed the title
num_annotations to js-config group context
12 tasks
marcospri
approved these changes
Sep 6, 2024
|
|
||
| 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 |
There was a problem hiding this comment.
I had trouble passing this parameter meaning.
It might be clearer with a more verbose name? include_only_me? Not sure
| @@ -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 | |||
There was a problem hiding this comment.
is unshared always going to be False? Or are you thinking ahead of an immediate need for other type of group?
| @@ -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 | |||
There was a problem hiding this comment.
is unshared always going to be False? Or are you thinking ahead of an immediate need for other type of group?
| @@ -193,6 +193,20 @@ def __call__(self, search, params): | |||
| ) | |||
|
|
|||
|
|
|||
| class SharedAnnotationsFilter: | |||
There was a problem hiding this comment.
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.
seanh
force-pushed
the
add-shared-mock-AnnotationStatsService
branch
from
4da3419
to
ffe4796
Compare
Base automatically changed from
add-shared-mock-AnnotationStatsService
to
main
September 10, 2024 09:28
seanh
force-pushed
the
add-num-annotations-to-jsconfig
branch
from
998570d
to
d71a933
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The frontend is going to need this in order to show a warning dialog when changing the type of a group: the warning only needs to be shown of there are shared annotations in the group. Also, the warning needs to include the number of annotations in the text. See #8898.
Testing
"num_annotations"in thejs-configin the HTML. You should see"num_annotations: 0""num_annotations": 1"num_annotations": 2"num_annotations"should not change"num_annotations"should decrease"num_annotations"should increase