Use IsaacSearchInstructionBuilder for events search endpoints#756
Use IsaacSearchInstructionBuilder for events search endpoints#756jsharkey13 merged 28 commits intomainfrom
Conversation
…vents Also fix some checkstyle warnings
This is never used (it's always hardcoded as false in the frontend), and the new Elasticsearch framework has no way of dealing with it.
To remove deprecated parameter
Move this logic to IsaacSearchInstructionBuilder to simplify building instructions in the EventsFacade
For consistency with current behaviour
jsharkey13
left a comment
There was a problem hiding this comment.
A lot of the logic added to the EventsFacade doesn't really belong in a Facade class.
I think part of the problem is there's no good example of how to do this right, at present. Throwing more methods into the GitContentManager has been used before, but isn't right either.
I think that we should make a new Manager class, EventsManager (with the other event-related managers in isaac.api.managers) to hold the business logic, and leave the Facade to do auth/caching and coercion of user-provided params into our Enums.
For that to work, I think the GCM needs a new method to provide a new IsaacSearchInstructionBuilder initialised with the default arguments (make nofilter content hidden by default here, and add a new method to ISIB like searchFor like includeHiddenContent(boolean) and I think avoid using nofilter on the public method name so we can finally move away from the silly tag). You can then customise this in the EventsManager without needing all the properties to be configured in multiple places.
Start by moving the logic for just the three methods you've refactored here down to a manager; don't worry about the rest for now. It may be that three methods in the manager, one per Facace method, is fine. But you may find some repeated logic can be grouped up into private manager methods too!
These are now set in the git content manager
isaac.api.Constants has exactly the same constant with the same value!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #756 +/- ##
==========================================
+ Coverage 39.92% 39.95% +0.02%
==========================================
Files 545 546 +1
Lines 23892 23887 -5
Branches 2873 2873
==========================================
+ Hits 9538 9543 +5
+ Misses 13444 13436 -8
+ Partials 910 908 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
These methods all use the deprecated BooleanSearchClause under the hood, and to ensure we don't continue using them or use them for new things, mark them as @deprecated.
src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventsManager.java
Outdated
Show resolved
Hide resolved
src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventsManager.java
Outdated
Show resolved
Hide resolved
Inadvertently falling through these if statements led to a null response being sent to the client rather than an error.
For some reason we always sorted in descending order, but then sometimes also requested ascending order. ElasticSearch seemed to cope with this, but it was confusing!
Causes "nofilter" events to be hidden from non-staff users.
Also requires frontend and test content updates (the latter to fix the test that's failing on this branch).