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 filters in notifications endpoint #267

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rodolfomiranda
Copy link
Collaborator

This PR address issue #266 . It adds to NotificationCollectionEnd three optional query parameters

  • read to filter by the read property of the note (true or false).
  • route to filter by the route r property in the attributes section (a) if exists.
  • order to order the results by datetime in asc or desc order.

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.11%. Comparing base (18d3ad7) to head (50543d8).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #267      +/-   ##
==========================================
+ Coverage   93.06%   93.11%   +0.04%     
==========================================
  Files          36       36              
  Lines        7121     7243     +122     
==========================================
+ Hits         6627     6744     +117     
- Misses        494      499       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lenkan
Copy link
Collaborator

lenkan commented Jul 5, 2024

Very helpful feature!

Question: I am not familiar with the notifier implementation, but this looks like it would load all notes in to memory, then filter them? Would it make sense to instead implement the filtering in keripy, utilizing the iterator to avoid loading all notes into memory?

@rodolfomiranda
Copy link
Collaborator Author

rodolfomiranda commented Jul 5, 2024

That's a good point. The optimal way is to implement it in keripy but it needs to run the iterator also on the countAll function. However, for the order part we still need to get them all in memory before pagination.
I can move it there easily, but I need the feedback from others specially @pfeairheller. If we are in agreement, I'll adapt this one and create a onother in keripy.

@rodolfomiranda
Copy link
Collaborator Author

I created a PR in KERIPY (WebOfTrust/keripy#816) that should be a more efficient way to implement the ordering and filtering.
I'll put this PR in draft until the keripy PR is being review. If it's merged, I'll adapt this one.

@rodolfomiranda rodolfomiranda marked this pull request as draft July 10, 2024 13:59
@pfeairheller
Copy link
Member

I've commented and requested changes in (WebOfTrust/keripy#816). We can readdress this issue when that is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants