This allows us to pass in dates with milliseconds. In tests, to convert a date to a json payload we do: date?.toInstant()?.toString() which includes the milliseconds. Without this change, it would fail to marshall dates on the backend to a Date field in a command object.

This seemed like usefull behaviour to support in general (it's a part of the ISO format), so I included it.

This was needed for tests since we want to be able to make asserts on the full object. It's only used in the new create product API, but we may want to consider using it more broadly since it's probably generally helpful to return all fields of the object.

I think we should have a separate discussion about how we want to jsonify our objects going forward. It'd be nice to have a standard that we can follow.

My rule was: include all fields and for all relationships, include the id so that the caller can look them up if they need to

I agree the id should be enough and this is probably what Grails does out of the box if you were to return the whole domain object (not the JSONed one, but the domain itself).
Nitpicky: maybe toFullJson() sounds better? In the same way like we have toBaseJson() for the Location probably.
Btw. since Grails returns only id for the associations, we might say that this toJson is not needed, but I am a big fan of DTOs and not returning domains to the representation layer, so I love the idea of either having "toJsons" for everything or DTO classes, and maybe consider something like MapStruct for such parsing.

and this is probably what Grails does out of the box if you were to return the whole domain object

You mean like if I did this:

[
    productType: productType
}

Wouldn't that pull the whole productType entity? Or maybe it'd be the equivalent of productType.toString() I can't remember.

I like toFullJson() I'll use that.

I am a big fan of DTOs and not returning domains to the representation layer, so I love the idea of either having "toJsons" for everything or DTO classes, and maybe consider something like MapStruct for such parsing

Yeah I think this is something we should look into further. In my previous job we had DTOs for every API and we'd either add the mapping directly to the DTO or use XConverter classes that would map a domain to a DTO for responses and a DTO to a domain for requests. We never used MapStruct but it seems like it could be a good solution.

This comment is about what I talk about here: https://pihemr.atlassian.net/wiki/spaces/OB/pages/3709894657/OBPIH-7395+Making+the+App+Testable#Expose-API-endpoints-for-public-service-methods

Essentially, in order for us to test the app, we need to make it more testable. IMO that should be done in multiple phases:

1. If some essential functionality doesn’t expose API endpoints (because it's still using the old GSP approach), write a simple XApiController for that functionality that does exactly what the old XController does. Don't refactor any of the logic yet.
2. Write API tests on the new endpoints
3. Once the API tests pass, refactor the service, splitting it up into smaller classes with the goal of making it more unit testable
4. Write unit tests on the new classes that were created

Repeat the above for other features.

So this PR is step 1 and 2. A future PR could revisit this controller and service to refactor the flow, break it up into helper classes, and unit test those classes.

The idea is that since we'll already have the API tests, we can be somewhat confident that our refactoring hasn't broken the core flow.

Then much later when we want to refactor the page to use React, we already have the APIs ready to go.

we have to be careful with that method though. it might not collect potential errors and exceptions in the same way like in the ProductController method.
The reason for that is that the ProductApiController should not be marked as @Transactional - we would want to handle the errors and commit the transaction in the service, and for this controller, we commit it at the end of the session, hence we might have some silent exceptions here.
Not something for you to refactor though, but wanted to point that out, that those methods might not act in the same way for some scenarios.

Good point. There's a lot of refactoring that needs to be done to this controller and we should keep an eye out for any strange behaviours relating to transactions and transaction rollbacks

This was just a bug fix that I discovered during testing. If we do a record stock in the current second, the adjustment will be 1 second in the future, and was breaking this flow.

are we sure it's a safe change? if we record stock in the current second we would like a baseline to be created in the current second, but the adjustment should be created one second later. I just want to make sure we don't break the reports with that change.

yes the adjustment will still be one second in the future. This change just stops it from erroring if we say "give me product availability at date x" where x is in the future. With this change, it'll return the current product availability.

Record stock is fetching availability at the given date + 2 seconds and since tests do record stock at now - 1 second (which results in the baseline being created at now -1 second and the adjustment being created now), it was trying to fetch availability at now + 1 which errored.

This is an example of an XApi class that you'd write when adding a new API that you want the tests to be able to call.

These classes are simple. There should be one method per API, and all they do is set up the request params and make the request.

what does it mean?

There should be one method per API

does it mean if I were to add e.g. a delete method I shouldn't add it here?

You could add it here. I just mean that this class shouldn't have a bunch of helper methods associated with that delete method. That's what the XApiWrapper class is for. This class is for the API calls, nothing else

See ProductApi.groovy for an example. There are a bunch of methods in there but only one per API

This is an example of an XApiWrapper class. You don't need to make these if you don't want to, but they can be used to enhance API calls with some convenience behaviour such as:

• allowing callers to pass objects as a request body (and then mapping them to json)
• automatically asserting on the response code
• binding response json to an object for callers to use

This is a good example of how the wrapper and test builder convenience classes are used. We build a Product using the test builder, then pass the Product to the create product API via the wrapper, which converts the product to json, makes the request, asserts that a 200 code is returned, then converts the json response to a Product.

It significantly simplifies operations that tests will need to do frequently.

given our convention, shouldn't it be /api/products that is reusable for all HTTP methods?
so in this case the POST would be responsible for save, so we could just add a method to 111th line.
Btw. parseRequest is not probably necessary anymore in Gralis 3.

I did it this way because the existing APIs are /product/save, /product/update... and I wanted to preserve the behaviour but now I realize there's no reason to keep that structure for the new APIs so yes you're right I'll do it that way

we have to be careful with that method though. it might not collect potential errors and exceptions in the same way like in the ProductController method.
The reason for that is that the ProductApiController should not be marked as @Transactional - we would want to handle the errors and commit the transaction in the service, and for this controller, we commit it at the end of the session, hence we might have some silent exceptions here.
Not something for you to refactor though, but wanted to point that out, that those methods might not act in the same way for some scenarios.

let's use types here

in the future we might consider refactoring the service method to validate the command while binding like we used to do recently in many places, so that we could swap the order of save and checking if the command has the errors.

Agreed. This is something we can look at when refactoring in the future

are you sure it did not refresh? it should refresh after each transaction save via event. Maybe you have run into an issue that you were editing the code and the events stopped being triggered?

I copied this from the old InventoryItemController. I assume this is done because the refresh is disabled in InventoryService: https://github.com/openboxes/openboxes/blob/develop/grails-app/services/org/pih/warehouse/inventory/InventoryService.groovy#L1477-L1480

I think this is something that we should address when refactoring record stock, which is why I left the behaviour as is for now.

I agree the id should be enough and this is probably what Grails does out of the box if you were to return the whole domain object (not the JSONed one, but the domain itself).
Nitpicky: maybe toFullJson() sounds better? In the same way like we have toBaseJson() for the Location probably.
Btw. since Grails returns only id for the associations, we might say that this toJson is not needed, but I am a big fan of DTOs and not returning domains to the representation layer, so I love the idea of either having "toJsons" for everything or DTO classes, and maybe consider something like MapStruct for such parsing.

are we sure it's a safe change? if we record stock in the current second we would like a baseline to be created in the current second, but the adjustment should be created one second later. I just want to make sure we don't break the reports with that change.

what does it mean?

There should be one method per API

does it mean if I were to add e.g. a delete method I shouldn't add it here?

I think it would be good to build the url factory in the same way like on the frontend, to be able to reuse the URLs or parts of them

I don't really like just having a huge urls file with all of them in there but you're probably right that at least it's better than hardcoding them everywhere. Maybe there's a smarter way to do it though such as breaking it up by feature.

I think it should be in something like "inventoryApiController", record stock sounds like a feature from a UI perspective, from the backend perspective, I would say it's related to managing inventories

it's an interesting thought. I don't necessarily think record stock is a UI feature but I see your point.

I wanted to avoid putting everything into a general inventoryApiController because the current inventoryController is 1500 lines and I'm scared we'd eventually end up with the same. Classes should have a clear purpose and IMO we should strive to draw a clearer line between feature boundaries.

If we want to treat "inventory management" as the base feature, I think we'd need to break it down further into sub-features.

So if I'm imagining a folder structure, it could be:

- inventory
    - cycleCount
    - recordStock
    - inventoryImport

which is why I went with /inventory/record-stock in the uri and why I created a recordStockApi as opposed to an inventoryApi. CycleCount has cycleCountApiController and inventory import has its own classes so why not the same for record stock?

Currently record stock is built in inventoryService, which is 3300+ lines of code. It's pretty hard to navigate a file that big and there's no separation of features. It's all in the same place. I think we should be trying to find ways that we can drive bigger lines between features and one piece of that is splitting out into more feature-specific controllers.

But we can discuss. I'm open to other ideas as well.

@alannadolny I'm going to merge this as is so that I can start using these test helpers, but this API is only used in tests right now so lets continue the discussion and we can easily refactor this if needed.