Codecov Report

Attention: Patch coverage is 68.75000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 7.74%. Comparing base (08ea84c) to head (ec15f10).
Report is 177 commits behind head on develop.

@@             Coverage Diff              @@
##             develop   #4996      +/-   ##
============================================
+ Coverage       7.72%   7.74%   +0.01%     
- Complexity       854     855       +1     
============================================
  Files            610     613       +3     
  Lines          42589   42621      +32     
  Branches       10346   10350       +4     
============================================
+ Hits            3292    3302      +10     
- Misses         38810   38825      +15     
- Partials         487     494       +7     

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

thanks for the heads up @alannadolny. I didn't know that was how we were populating our filters. The SelectOptionsApiController methods need an ID like you said but the ABC classes are just plain strings (there's no ProductClassification domain, we search columns of multiple tables to build the list) and so they don't have an id. I'll need to dig into how its working on the frontend

EDIT: I misunderstood what Alan meant by class. I think he's right, we can use the ABC class name as the id. That's what the client will pass to the server for filtering.

ABC classifications had to be an option for selects. We are using SelectOptionsApiController so it should be placed in this controller and mapped in the same way as others (to have an ID and label).

I would like to get rid of SelectOptionsApiController at some point. I argued against it when it was first introduced but was overruled at the time. But I still want to refactor it out of the API as there seems to be no reason for a consolidated API that applies a different rendering of the same data returned by existing APIs. And if the argument is "well we don't have an API for X yet", then we should add an API for X instead of another listing method to SelectOptions.

The client should be responsible for mapping a response to the UI, not the API. In other words, if I need a list of category options I should just call the Category API from the client and let the client deal with mapping / filtering the values properly? If we absolutely need the API to return a different representation of the category response, then we should use content negotiation (i.e. Accept header) or format parameter. If we want to reduce the size of the response, we can implement response filtering (i.e. /api/categories?fields=id,name)

I ran into the same issue when we introduced the JQuery data tables plugin. I needed to conform the API response to allow the plugin to render the data table properly. And it made me feel dirty.

The SelectOptionsApi is slightly different in that we control the shape and content of the response, but we're still telling the server to do something it shouldn't need to do. And if we do need to tell the server to do something different re: rendering data it should be through a convention like content negotiation.

I tend to agree with Justin's comment. Our APIs shouldn't contain frontend-specific logic.

Likely the "correct" way to achieve this is via a list API that allows the client to optionally specify what fields it needs via query parameter.

That said, in this case I can see how it'd be useful to have a standardized API response format for when we need to simply enumerate all of something and don't want to return the entire object. It feels like the path of least resistance here is sticking with the format used in SelectOptionsApiController because the client logic is already expecting that format. It's generic enough for me to be okay with it. I don't think this API should be in the SelectOptionsApiController though. I think it should live in the feature-specific ProductClassificationApiController. From there I can standarize this type of "enumeration API" by using a common response dto of the format:

[
  {
    'id':  ??
    'label': "A"
    'value': ??
  },
  ...
]

I've added the frontend logic. I decided to stick with the original API and construct the expected format on the client side so that the API can stay flexible and frontend agnostic.

The only thing I haven't figured out is how to get the filters to actually apply when searching. Right now on the client it sends:
/openboxes/api/facilities/xxx/cycle-count-candidates?abcClass=%5Bobject%20Object%5D&... (which is "[object Object]") so it's clearly not parsing the classification name correctly somewhere. I just need to figure out where (I'm not well versed in debugging the frontend...)

I'm merging this since most of the new changes are to get integration tests working or addressing existing comments but feel free to leave further comments if you want.