OBPIH-7113 dynamic sorting on receiving locations by ewaterman · Pull Request #5282 · openboxes/openboxes

ewaterman · 2025-05-27T20:40:36Z

✨ Description of Change

Link to GitHub issue or Jira ticket: https://pihemr.atlassian.net/browse/OBPIH-7113

Description:

Video recording describing the PR: https://jam.dev/c/b2a026dc-56f9-4abc-9b9d-932cf401081f

Adds the ability to provide the "sort" query param when fetching internal locations. The param is a comma separated list of fields and you can put a '-' in front of a field to have it sort descending. For example: "?sort=x,-y" means sort ascending by x then descending by y.

I achieved this by adding a new SortParam class. We can bind the "sort" query param to a list of SortParam and then use SortUtil to easily sort a list of objects.

So for example, if we want to sort Locations by order then by name, both ascending:

List<Location> locations = [
    new Location(id: 0, order: 0, name: "Z"),
    new Location(id: 1, order: 1, name: "A"),
    new Location(id: 2, order: 0, name: "A")
]
SortParamList sortParams = [
    new SortParam(fieldName: "name", ascending: true)
    new SortParam(fieldName: "order", ascending: true)
]
List<Location> locationsSorted = SortUtil.sort(locations, sortParams)
assert locationsSorted[0].id == 2
assert locationsSorted[1].id == 0
assert locationsSorted[1].id == 1

And to use it in an API:

client does a GET /api/locations?sort=order,name

we bind that to a command object:

class LocationCommand {
    SortParamList sort
}

then in the service:

List<Location> getLocations(LocationCommand command) {
    List<Location> locations = Location.find(...)
    return SortUtil.sort(locations, command.sort)
}

Note that this should only be used when we can't sort in the query itself since doing it in the query is typically faster.

I was originally going to implement this as the ticket suggested and just have the default sort behaviour be to sort by location type but I was worried about the changes it would have on other places in the code so I ended up implementing the full solution.

📷 Screenshots & Recordings (optional)

https://jam.dev/c/bd9c2d59-1a28-4830-aede-4336923bf636

grails-app/controllers/org/pih/warehouse/api/InternalLocationApiController.groovy

ewaterman · 2025-05-27T20:46:43Z

grails-app/controllers/org/pih/warehouse/api/InternalLocationApiController.groovy

-        activityCodeParams.allActivityCodes = params.allActivityCodes ? params.list("allActivityCodes") as ActivityCode[]: []
-        activityCodeParams.anyActivityCodes = params.anyActivityCodes ? params.list("anyActivityCodes") as ActivityCode[]: []
-        activityCodeParams.ignoreActivityCodes = params.ignoreActivityCodes ? params.list("ignoreActivityCodes") as ActivityCode[] : []
+    def list(InternalLocationSearchCommand command) {


I didn't originally implement this with a command but locationService.getInternalLocations was so annoying to work with when activityCodeParams was a Map because I needed optional params and the compiler was complaining that it couldn't distinguish between methods so I got fed up enough to refactor it.

codecov · 2025-05-27T20:48:47Z

Codecov Report

❌ Patch coverage is 33.01887% with 71 lines in your changes missing coverage. Please review.
✅ Project coverage is 8.42%. Comparing base (77c5d04) to head (5aad23b).
⚠️ Report is 168 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ices/org/pih/warehouse/core/LocationService.groovy	0.00%	16 Missing ⚠️
...warehouse/api/InternalLocationApiController.groovy	0.00%	15 Missing ⚠️
...s-app/utils/org/pih/warehouse/sort/SortUtil.groovy	63.41%	10 Missing and 5 partials ⚠️
...arehouse/core/InternalLocationSearchCommand.groovy	0.00%	11 Missing ⚠️
...rehouse/core/ReceivingLocationSearchCommand.groovy	0.00%	6 Missing ⚠️
.../domain/org/pih/warehouse/core/LocationType.groovy	0.00%	3 Missing ⚠️
.../utils/org/pih/warehouse/sort/SortParamList.groovy	62.50%	3 Missing ⚠️
...use/databinding/SortParamListValueConverter.groovy	33.33%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             develop   #5282      +/-   ##
============================================
+ Coverage       8.34%   8.42%   +0.08%     
- Complexity       995    1025      +30     
============================================
  Files            647     656       +9     
  Lines          43466   43829     +363     
  Branches       10539   10612      +73     
============================================
+ Hits            3626    3694      +68     
- Misses         39278   39562     +284     
- Partials         562     573      +11

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

awalkowiak

Overall feels to me that the sort util is redundant here, and I'd just stick to command + criteria builder or command + sort

awalkowiak · 2025-06-09T13:08:23Z

grails-app/utils/org/pih/warehouse/sort/SortParam.groovy

+/**
+ * A POJO for holding a sorting condition on an entity.
+ */
+class SortParam {


Why POJOs instead Command objects?

it's used by command objects (See the new ReceivingBinSearchCommand). Idk if that makes it a command itself but probably it's just a semantics thing. If you prefer, I'm fine with naming these SortParamCommand and SortParamListCommand if that is clearer

awalkowiak · 2025-06-09T13:08:41Z

grails-app/utils/org/pih/warehouse/sort/SortParamList.groovy

+/**
+ * A POJO for holding a list of sorting conditions on an entity.
+ */
+class SortParamList {


Same quesition here with the command as above

awalkowiak · 2025-06-09T13:17:55Z

grails-app/utils/org/pih/warehouse/sort/SortUtil.groovy

+     *
+     * @param sortString A string of the form "x,-y,z"
+     */
+    static SortParamList bindSortParams(String sortString) {


Could this be achieved by auto-binding with a command? Do we need to add a util for it?

It actually is auto-binding. SortParamListValueConverter is where the logic is. All it does is call this method so we could probably just move this method there if we don't want to expose the behaviour to the rest of the app

awalkowiak · 2025-06-09T14:34:13Z

grails-app/utils/org/pih/warehouse/sort/SortUtil.groovy

+    /**
+     * Sorts a given list of entities using the provided sort parameters.
+     */
+    static <T> List<T> sort(List<T> toSort, SortParamList sortParams) {


Is there a case that will utilize it? Feels like this is basically achieving exactly the same what command + criteria builder would do or command + .sort { }

it's used in LocationService. In that code, we've already fetched from the db and so have a List that we need to sort. So yes we're essentially doing the equivalient of List.sort(), it's just for convenience so that we can simply provide a SortParamList to sort by (which is provided by the user via the ?sort=x,y,z query param.

The big thing is that it allows us to dynamically sort by a user-given list of params, instead of a fixed set of params defined in the command object that List.sort() would do.

We could also make a new util method that would take in a SortParamList and output criteria that we can use to query the db with.

jmiranda · 2025-06-12T04:12:07Z

src/main/groovy/org/pih/warehouse/core/ReceivingBinSearchCommand.groovy

+/**
+ * Used to filter the returned results when listing/searching internal receiving bin locations.
+ */
+class ReceivingBinSearchCommand extends InternalLocationSearchCommand {


I know the term "receiving bin" has been used a lot throughout the system and maybe in requirements, but I would love to get rid of the notion of a "receiving bin" once and for all. It's more appropriate to call these locations "receiving locations". It's usually not a bin in the typical sense of a "storage bin" or "bin location". It's a staging location on the floor of the warehouse near the dock door.

Location 02 is the receiving area.

…rchCommand

Co-authored-by: Walkowiak <[email protected]>

dynamic sorting on receiving locations

97c645e

ewaterman self-assigned this May 27, 2025

github-actions bot added type: feature A new piece of functionality for the app domain: frontend Changes or discussions relating to the frontend UI domain: backend Changes or discussions relating to the backend server labels May 27, 2025

ewaterman commented May 27, 2025

View reviewed changes

grails-app/controllers/org/pih/warehouse/api/InternalLocationApiController.groovy Show resolved Hide resolved

ewaterman commented May 27, 2025

View reviewed changes

ewaterman requested a review from awalkowiak May 27, 2025 20:48

ewaterman requested a review from kchelstowski May 27, 2025 20:48

fix receiving query bug and simplify using SortParamList

3954a8d

awalkowiak requested review from alannadolny and jmiranda May 30, 2025 13:44

ewaterman requested a review from SebastianLib June 2, 2025 17:59

awalkowiak reviewed Jun 9, 2025

View reviewed changes

jmiranda reviewed Jun 12, 2025

View reviewed changes

OBPIH-7113 Rename ReceivingBinSearchCommand into ReceivingLocationSea…

5aad23b

…rchCommand

awalkowiak approved these changes Jun 12, 2025

View reviewed changes

awalkowiak merged commit 1b1d300 into develop Jun 12, 2025
8 checks passed

awalkowiak deleted the ft/OBPIH-7113-receiving-bins-in-cc branch June 12, 2025 15:38

alannadolny pushed a commit that referenced this pull request Jun 17, 2025

OBPIH-7113 dynamic sorting on receiving locations (#5282)

d65ec31

Co-authored-by: Walkowiak <[email protected]>

Uh oh!

Conversation

ewaterman commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ Description of Change

📷 Screenshots & Recordings (optional)

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

awalkowiak 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!

ewaterman Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmiranda Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ewaterman commented May 27, 2025 •

edited

Loading

codecov bot commented May 27, 2025 •

edited

Loading

ewaterman Jun 9, 2025 •

edited

Loading

jmiranda Jun 12, 2025 •

edited

Loading