@jmiranda I tried to follow your last suggestion that those params should be also bound to a command, but suddenly they aren't 🤔 they are only accessible via params.

For what it's worth, it did work for me. I'll try to pull up the PR that I was working on to see if I did anything differently. Off the top of my head, the only difference I can remember is that the UrlMapping I used had braces around facility (I remember because Evan commented on it). I doubt that has any bearing since $facility and ${facility} should be equivalent, but worth a shot I guess.

"/api/facilities/${facility}/cycle-counts/start/batch"

Ok, I just tested this again and the command data binding works for GET requests, but not for POSTs. Interesting and annoying.

Edit: Here's a PR with a simple data binding for the major verbs
https://github.com/openboxes/openboxes/pull/5021/files

and a short video showing the behavior
https://app.screencastify.com/v2/manage/videos/1jekeZ1yqCH0okuRP7Ia

@jmiranda thanks for testing it out. Yeah it's annoying. Btw - the link with the video doesn't work for me.

Sorry had to change the permissions on it. Should be accessible now.

(I see the typo)

I followed this:

I'll let Artur review these first. I haven't spent much time thinking about what they should look like

as mentioned in the FIXME - I am not sure what default values for those should be, as they weren't determined anywhere in the docs.

custom: false for all rows initially. It should only be true for rows that the client adds manually (the last row in this image). We need to distinguish these rows because for those the user is allowed to specify bin location and lot number.

I think draft: false for all rows initially makes sense (because we don't even have a draft yet, just items that are ready to count), though maybe draft: true is fine. The main use for it is in computing CycleCount and CycleCountItem status. If the user adds a count for all rows but doesn't submit it yet (so draft: true for all rows) we want to make sure that we keep the status as COUNTING (and don't advance it to COUNTED yet).

Thinking about it more now though, maybe the fact that we haven't called submit count yet will already guarantee that status==COUNTING and so we might not even need the draft flag at all.

tech huddle: remove the draft property

Should it be removed (i still see it)?

in the doc it was meant for "If the cycle count is “resolving” or “counted” - since we don't have RESOLVING status I assumed this is equivalent to the COUNTING? 🤔

we need to be able to distinguish between:

1. a cycle count exists and the current count has already been started (status in [COUNTING]) so just fetch it
2. a cycle count exists and the previous count was completed (status in [COUNTED]) so we're starting a recount

(I'm not totally sure where INVESTIGATING fits into those cases but it's in one of them. If INVESTIGATING means a recount is already started, then it's case 1. If it means a recount is required but hasn't started, then it's case 2.)

Case 2 can be left for TODO for now because it's recounting but we need to properly deal with 1.

There's two was to handle case 1 in this API:
A) Throw an error. You're not allowed to start a count that has already been started.
B) Simply return the cycle count and its items as they are because the count has already started / been initialized

If we pick A, then the frontend will need to do two things when starting a batch count:

• Call a GET list API to fetch all cycle counts in the batch that already started
• Call this POST start count API on all requests that haven't been started yet

The design was done assuming solution B because it would be simpler for the frontend. This means this API needs to be able to gracefully handle both 1 and 2 cases.

Let me know if that makes sense. And if you feel like solution A makes more sense (which after seeing how complex this flow already is, I'm wishing was what I picked originally) we can talk about switching to that but unless you say otherwise I'll assume you want to stick with the original plan (aka B).

could you elaborate on that?

a cycle count exists and the current count has already been started (status in [COUNTING]) so just fetch it

I kinda don't get what the current count means at this point. If we update a cycle count, it means that you've just fetched it, and it's our "existing cycle count" and "current count" at once.

• You're on the TO COUNT tab and start a count on request_id == 1.
• Frontend calls the start count API and the backend creates and returns cycle_count_id == 1.
• You log out and come back to the TO COUNT tab and click start count again on the same request (1)
• Frontend calls the start count API (again). The fetches the count associated with that request, finds it, sees that it's active, and so just returns it (so the start count API is essentially doing the get on behalf of the frontend)

an alternative flow (that I wish I designed instead) would be:

• You're on the TO COUNT tab and start a count on request_id == 1.
• Frontend calls the start count API and the backend creates and returns cycle_count_id == 1.
• You log out and come back to the TO COUNT tab and click start count again on the same request (1)
• Frontend sees that the count is already in progress (based on the returned status) and so calls GET counts with id=1 (so the frontend is responsible for knowing what requests to start, and what counts to get)

tech huddle: COUNTING -> INVESTIGATING

this was meant to work for cycle count "in progress", so I took this from:

"If the given countIndex != max(count_index of the Items) , error"

(Will need to test it deeper though, as all my data contained only one item, hence I am yet sure if I should look for first or last item after sorting)

I think it sorts low to high so it'd be last? I'm not sure. You could also just use Collections.max() if you don't want to bother sorting the whole list of items

I gotta say it was quite tricky to work with the items without having the bidirectional relationship between Cycle count and Cycle count items, but I remember the reasons for promoting the unidirectional many-to-one relationship.

this is one of the "tricky" parts I mentioned above. If I had an access to the list of the items from the cycle count domain, I wouldn't have to do that.

eventually we can check if every item has the same cycle count (in case in the future this method would be used for "mixed" cycle count items (with different cycle count associated) and throw an exception.
Now, it can't happen, so let me know your thoughts.

here's the docs I made forever ago talking about relationships.

I think it could actually be fine here because:

• The number of CycleCountItems for each CycleCount should be minimal (ie <100)
• It's not an ever growing list of items. We do a count, maybe do a recount, then we're done. The cycle count is eventually resolved and we stop adding items to it
• Typically when fetching a CycleCount we already fetch all of its Items. The biggest concern of the bidirectional was that adding a new CycleCountItem would require fetching and validating against all existing ones, but since we should already have all Items loaded, it's not really a problem

So maybe it's fine in this case to allow for bidirectional.

Or if you still don't want to do bidirectional, then I think this is fine. You could also pass in the CycleCount as a method arg here instead of extracting it from the Items. That feels more natural to me since you're building a DTO by giving it all the parts it needs.

As an aside, if we're not doing bidirectional but still want to be able to get all items for a cycle count, we could just have the CycleCount domain implement a getCycleCountItems() method that does a CycleCountItem.findAllByCycleCount(cycleCount) call.

since the cycle count request's status is a persistent value, I'm wondering if we should change it right here, if so - should it be REQUESTED or IN_PROGRESS?

IN_PROGRESS makes sense to me here but I'll defer to Artur

iirc this should be derived, so we can disregard saving it (at least for now)

note that this will change when we introduce recount since starting a recount will use the same start count API. https://pihemr.atlassian.net/wiki/spaces/OB/pages/3363340321/TO+RESOLVE+page#Backend

does the client need to pass the whole request here or just request id and Grails binds the request object to it?

we had that discussion recently regarding the Product.
Both options are possible, so to provide it like:

{
  "cycleCountRequest": "UUID"
}

or

{
  "cycleCountRequest": {
    "id": "UUID"
  }
}

now that I'm seeing this, I wonder if the dateLastRefreshed field would be better off in the CycleCountItem because that's where we're doing the actual QoH refresh. If we're in a recount and do a refresh, we only change QoH for the CycleCountItem with countIndex==1 (countIndex==0 stays unchanged). So it could be useful to have that date info at the item level to be able to clearly see why QoH changed between counts.

I don't know if you've had a chance to think about this yet. (For context, this is the API that will use the field)

tech huddle: this could be added to the cycle count item later

Does Grails default to nullable: false for all fields? I always thought it was the other way around

yes, nullable: false is the default value.
source: https://docs.grails.org/latest/ref/Constraints/nullable.html

I'm realizing that we never had a group discussion on what values should exist here (I wrote these ones myself). Lets discuss it with Manon.

We'll also need to localize each of these.

DiscrepancyCause -> DiscrepancyReasonCode

Is the idea here that you don't need all fields so "basic" means a simplified DTO and for the other APIs we'll have a more complete DTO that returns all fields?

I generally like to be specific with the naming of DTOs since we could get to a place where one API needs one format, and another needs another format, and so on, and so if we have a DTO named "basic", it becomes unclear what API it should and should not be used for.

That said, I'm fine with it for this use case because I can't think of a better name.

But also maybe it's fine to always just return the full CycleCountItemDto with all its fields

I followed the response pattern designed here.
Let me know if this makes sense or we would want to return the whole cycle count item.

hmm yeah I don't know if there was a good reason why I left out fields from the responses. Probably I just included the important ones. Sorry for not being clear about it. I'll leave it for you to decide but I think we'd be fine with just one CycleCountItemDto for all our responses here.

EDIT: ah now I remember. I think I left the other fields out in the example response because it's implied that they'd be null. It looks like I missed a couple fields though. Probably because the domain fields were changing a lot at time time. Apologies.

here's the docs I made forever ago talking about relationships.

I think it could actually be fine here because:

• The number of CycleCountItems for each CycleCount should be minimal (ie <100)
• It's not an ever growing list of items. We do a count, maybe do a recount, then we're done. The cycle count is eventually resolved and we stop adding items to it
• Typically when fetching a CycleCount we already fetch all of its Items. The biggest concern of the bidirectional was that adding a new CycleCountItem would require fetching and validating against all existing ones, but since we should already have all Items loaded, it's not really a problem

So maybe it's fine in this case to allow for bidirectional.

Or if you still don't want to do bidirectional, then I think this is fine. You could also pass in the CycleCount as a method arg here instead of extracting it from the Items. That feels more natural to me since you're building a DTO by giving it all the parts it needs.

As an aside, if we're not doing bidirectional but still want to be able to get all items for a cycle count, we could just have the CycleCount domain implement a getCycleCountItems() method that does a CycleCountItem.findAllByCycleCount(cycleCount) call.

I'll let Artur review these first. I haven't spent much time thinking about what they should look like

custom: false for all rows initially. It should only be true for rows that the client adds manually (the last row in this image). We need to distinguish these rows because for those the user is allowed to specify bin location and lot number.

I think draft: false for all rows initially makes sense (because we don't even have a draft yet, just items that are ready to count), though maybe draft: true is fine. The main use for it is in computing CycleCount and CycleCountItem status. If the user adds a count for all rows but doesn't submit it yet (so draft: true for all rows) we want to make sure that we keep the status as COUNTING (and don't advance it to COUNTED yet).

Thinking about it more now though, maybe the fact that we haven't called submit count yet will already guarantee that status==COUNTING and so we might not even need the draft flag at all.

IN_PROGRESS makes sense to me here but I'll defer to Artur

what happens if itemsToSave has > 1 element and we fail on the 2nd+ element? Will the 1st still be saved? That could put us in a weird state right? Maybe better would be to build them all, validate them all, then save them all

or maybe because it's all wrapped in the same @Transactional it's fine because it won't get committed.

Yes, since it's being persisted in the same transaction, an error in any of the items would cause the rollback of whole operation

I don't think lastUpdated is what you want here. That field should only ever be used for audit purposes. If someone goes back and edits an old cycle count for whatever reason, that field will get updated and so we'll fetch it here, even if there's a more recent cycle count.

I think what we want is a dateCompleted/Submitted (or whatever) field in CycleCount that gets set once the cycle count is submitted and is never updated again.

Also, you're fetching all counts on the product then in the code throwing out all returned rows exept the most recent. Is there a way to move that into the query itself? Something like a SELECT * WHERE MAX(dateCompleted). That would save us from having to select more than we need so it'd be less work on the db with fewer rows locked

Also, you're fetching all counts on the product then in the code throwing out all returned rows exept the most recent. Is there a way to move that into the query itself? Something like a SELECT * WHERE MAX(dateCompleted). That would save us from having to select more than we need so it'd be less work on the db with fewer rows locked

It's probably doable via the createCriteria, but for now I focused on delivering the easiest version possible, but I could try to improve the query

that's fine with me (though I encourage you to spend a bit of time trying to get it to work). If you're not going to fix it here though can you add a TODO comment for it? It could be a relatively important optimization because as we have more and more cycle counts on a product this will fetch more and more data, and so will only get worse over time.

can quantityOnHand be < 0? If so, we definitely want to count negative quantity items. Maybe an == 0 check is safer here.

also I think the condition is more complicated here. We want to exclude items where in the previous count QoH==quantity counted==0.

So we want include all rows that:

• have a non-zero QoH OR
• have a zero QoH but in the previous count QoH != quantity counted OR quantity counted != 0

filter out all bin locations + inventory items where QoH==0 AND (the previous cycle count did not include a count on that bin locations + inventory items OR the previous count also had QoH==0).

The quote above is taken from the tech design. Is it still applicable or the version you've just written is up to date?

yes it's still applicable. In the second half of your OR, you're including items that were in a previous count, but you only want to include them if quantity counted != 0.

In my comment above I thought it'd be good to further refine this to also include them if quantity counted == 0 but QoH at the time was != 0. The idea being that even though there was zero last time, there was a discrepancy so we should double check out.

Lets talk about it with Manon in the tech huddle though to confirm the exact expected behaviour. Maybe since we're already auto-filling a 0 for empty bins it's fine to not filter them out.

we need to be able to distinguish between:

1. a cycle count exists and the current count has already been started (status in [COUNTING]) so just fetch it
2. a cycle count exists and the previous count was completed (status in [COUNTED]) so we're starting a recount

(I'm not totally sure where INVESTIGATING fits into those cases but it's in one of them. If INVESTIGATING means a recount is already started, then it's case 1. If it means a recount is required but hasn't started, then it's case 2.)

Case 2 can be left for TODO for now because it's recounting but we need to properly deal with 1.

There's two was to handle case 1 in this API:
A) Throw an error. You're not allowed to start a count that has already been started.
B) Simply return the cycle count and its items as they are because the count has already started / been initialized

If we pick A, then the frontend will need to do two things when starting a batch count:

• Call a GET list API to fetch all cycle counts in the batch that already started
• Call this POST start count API on all requests that haven't been started yet

The design was done assuming solution B because it would be simpler for the frontend. This means this API needs to be able to gracefully handle both 1 and 2 cases.

Let me know if that makes sense. And if you feel like solution A makes more sense (which after seeing how complex this flow already is, I'm wishing was what I picked originally) we can talk about switching to that but unless you say otherwise I'll assume you want to stick with the original plan (aka B).

I think it sorts low to high so it'd be last? I'm not sure. You could also just use Collections.max() if you don't want to bother sorting the whole list of items

change to "READY_TO_REVIEW"

Should this be changed?