in the design, I had the URL like:

/api/receiving/v2/${shipmentId}/start

but while working on the ticket itself I realized that having it like that, might be a bit confusing, since the URL itself might indicate it is a receipt's id, not shipment's, so I also included /shipment
I hope you agree with me @ewaterman since you pointed out a similar problem in one of the other tickets.

nitpick: the URI is /receiving but the controller is ReceiptApiController. I think we should be consistent.
Either:

/receipt and ReceiptApiController
/receiving and ReceivingV2ApiController (I assume the V2 is needed to avoid having two controllers with the same name)

1 is nice because it is consistent with the "shipment" part of the URI. We always operate on objects. We're starting a "reciept" that is based on a "shipment".
2 is about the feature. The feature is "receiving" v2, and under that feature we are starting a receipt on a shipment. It is also nice because we already have the /receiving package that we can re-use here to keep all the files in the same place.
I'm fine wither either but I do think we should try to be consistent.

EDIT: I see that we are not consistent about it already so 🙃

Maybe I'm convinced that "receiving" is the feature/package and "receipt" is the object we're working on. 🤷

I was following the /partialReceiving, but after your comment I'm probably convinced we should follow the /receipts approach.
Not /receipt, but /receipts, as we have all of the URLs plural (cycle-counts, stocklists etc).

I would like to start using @Transactional like that. Previously we didn't care about the details of transactional annotation, and just marked all services as transactional, but for read only methods we don't need dirty checking etc, so we can save some memory on that, and mark the @Transactional (without read only) only the methods, that actually persist the data.

I think it's a good idea. Would it be safer to do the opposite though? Mark the service as regular @Transactional and then read-only methods as such? If it errors very obviously when you try to call save and readOnly = true then I'm fine with your approach. As long as devs wont get confused about what to do in that scenario

It would for sure make us lazier. I would prefer to have to remember adding @Transactional on persist methods (otherwise we get an error), rather than "if you forget adding readOnly, it will still work".

those might feel like form over substance, but I would like to stay consistent and have everything typed, instead of having plain maps even if the DTOs have only one/two fields.

that makes sense but since it is just an id field it does feel somewhat strange. Why not just make ReceiptDto have an orderId field? Is the idea that we might need to include other fields from the order in the future?

I think we don't have a clear convention around that, we have a mix of orderId vs order: [ id:, ...].
I personally prefer the nested way, because if we ever need to add more fields of order, we just add it to order's dto, instead of ending up either with a need of refactor the API response from orderId, to the map (which might be dangerous), or have to have ugly fields like orderId, orderName etc.
If I remember correctly, I mentioned in the design that I prefered the nested map/DTO approach rather than plain fields (orderId).
Let me know your thoughts

my opinion generally is that we should try to make more lightweight APIs. We're often making our APIs so that they contain every piece of information that a specific frontend page needs. This is convenient but it makes our APIs extremely inflexible. They can only really be used in that one case. It also means we're stuck making tons of different DTOs all over the place because each API needs to return slightly different data.

So a simpler approach: If we have a GET Receipt API, that should return the receipt, nothing else. It can return references to other objects (such as orderId) but it shouldn't need to return the actual fields of the order. If the frontend needs more information about the order, it should use the id that we return and call a GET Order API.

That might seem needlessly more complex, but if we start doing that more with our APIs, then the next time we build a frontend that needs to display a receipt, the frontend can reuse the API because it is general purpose.

I see it in the same way that we're trying to build components on the frontend. The backend shouldn't construct hyper-specific APIs that fit a specific frontend. Instead the backend should create generic APIs that the frontend can use alongside other generic APIs to piece together its own frontend.

This will make supporting multiple frontends (think mobile) waaaay easier because we won't need to also have APIs that return data specifically for each new frontend.

That said, I want to spend some more time thinking about this specific scenario with receipts because I do think it's more complicated. I'll try to reply here after I gather some thoughts

okay I have two thoughts:

1. why do we even need order and requisition in the ReceiptDto? What fields from those are going to be used?

2. my preference is to have a structure like I suggested in my previous comment:

ReceiptDto {
    shipmentId: 123,
    ...
}

and then the client can call GET /api/shipment/123 to fetch the shipment details. It keeps our APIs focused on one thing (the receipt) which makes it reusable. And we don't need to worry about if we need additional fields from the shipment because it is handled by its own dedicated API.

That said, I understand that can be annoying to deal with, especially for the frontend guys, so ultimately I am okay with putting the shipment itself in the response.

I'm also happy to deal with this in my own PR when working on the GET api since I'll be modifying the DTOs anyways.

Yeah, I think it's a fair point, that our APIs are usually overloaded. It would definetely be a good practice to just call small APIs instead of having a huge response that is needed mostly for frontend.
The only blocker I can see is that if we have facilities with slow connection, calling 5 requests instead of 1 would be costly.
I will stick with my current version for now and will let you play around with it, just not to block you with the read endpoint, and I will keep my fingers crossed for your discovery and output.

that's a fair point. Sometimes we can deal with that slowness by having the client make requests in parallel (we fetch the receipt, then make parallel requests to fetch the shipment, order, requisition etc...) so it hopefully wouldn't be so bad.

I still have my first question:

why do we even need order and requisition in the ReceiptDto? What fields from those are going to be used?

But anyways I'm fine with leaving it as is for now.

https://www.rfc-editor.org/rfc/rfc9110.html#name-201-created

The 201 response content typically describes and links to the resource(s) created.

If I'm interpreting that right, then the spec for code 201 says that we should not return the full created object in a 201 response, just an id or a URL so that the client can fetch the resource that was created.

We're not especially diligent about following the spec though, so ultimately I'm fine with how you have it. Maybe one day we'll develop a standard

that's interesting. I have always been taught that POST method of a simple CRUD (which indeed it is, as we create a receipt), should return 201 🤔

I don't have a problem with it returning 201, I just mean that the spec says we shouldn't also return the data we just created, we should return an id / url and the client should use that to fetch it.

but ultimately I don't think it's a big deal so the way you have it is fine

Yeah we had this discussion awhile back when we were building the APIs for the stock movement workflow. I was in favor of returning a Location header with the 201 response and requiring the client to fetch. Artur (or Pawel?) was in favor of returning the object in the response. I was overruled given the need to fetch the object almost every time we created a new one and I think that was the right decision at the time.

Fwiw, I think we should be returning the Location header to be compliant with the HTTP spec, but that would be something we should do across the board.

The way you have it is fine for now, but FYI I'm starting to work on a converter/mapper structure that allows us to break out this toDto logic to a separate class.

https://github.com/openboxes/openboxes/pull/5951/changes#diff-e8b253328bca95d2ca649c12a92b3c8f4cc025af586c2b7ff54dd573fab31769

I'm hoping we can eventually have a structure that allows us to do something like:

ReceiptDto receiptDto = objectMapper.map(someReceipt, ReceiptDto.class)

and internally it'll resolve some ReceiptObjectMapper component and call it.

but until that is implemented this works

yeah, this is part I forgot to let you know in DM when you asked me about the read endpoint.
I'm fine with you playing around with it and eventually refactoring it when you work on the read endpoint ticket.

that makes sense but since it is just an id field it does feel somewhat strange. Why not just make ReceiptDto have an orderId field? Is the idea that we might need to include other fields from the order in the future?

I suppose this is fine but it feels a little weird and we're really inconsistent about this. I don't think we do this in cycle count flows for example

I think the reason we don't do it there, is that we use plain maps, not DTOs and I wanted to have it as typed, as possible, so that whenever we have to access some of the DTO fields, we always know what the expected type is.
Probably this part is form over substance, as I don't suppose we will change the Person.toJson in the future, but if we do so, we are somewhat "safe".
Let me know if I should revert that or if I convinced you.

Yeah I generally prefer your approach.

The fact that we're chaining dto conversions, and that the Person.toJson() method is fetching a config is a good example to me what the benefit of the XMapper components would be. We could have a component like:

class PersonMapper implements Mapper<Person, PersonDto> {
    ConfigService configService

    PersonDto map(Person person) {
        boolean configService.getProperty("openboxes.anonymize.enabled", Boolean)
        return new PersonDto(...)
    }
}

so that we can keep the logic out of Person. Then do the same for receipt:

class ReceiptMapper implements Mapper<Receipt, ReceiptDto> {
    ReceiptDto map(Receipt receipt) {
        return new ReceiptDto(
            recipient: mapper.map(receipt.recipient, PersonDto)
            ...
        )
    }
}

but for now it's fine to leave it as you have it. I haven't implemented the logic yet that would let us do mapper.map(receipt.recipient, PersonDto)

As I mentioned above, feel free to play around with it in the read endpoint.

I think it's a good idea. Would it be safer to do the opposite though? Mark the service as regular @Transactional and then read-only methods as such? If it errors very obviously when you try to call save and readOnly = true then I'm fine with your approach. As long as devs wont get confused about what to do in that scenario

can't origin, destination, and dateShipped be in ReceiptShipmentDto?

how would you imagine accessing them further? via receipt.shipment.origin? I'm fine with that but I wanted to stay close to the previous PartialReceipt dto that had those fields separated like I have here.

yeah I was imagining via the shipment. It seems needless to duplicate the fields if we have them in another object. If we're going to have a ReceiptShipmentDto then we might as well use it right?

IMO I would just let the client call GET Shipment to get these fields and then simply pass the shipment id in the response that it can use

Shouldn't this be an enum? PartialReceiptStatus or ReceiptStatusCode? Or a new one if you have other ideas

good point, I forgot about it.

Looks like this disables the RefreshOrderSummaryEvent. Why is this needed?

I don't have the answer for that, but it is done like that in the previous version, so I didn't want to change that behavior:

I wont force you to do it, but I do think it's worth figuring out. Even though it is not new logic we're adding it to new APIs so we're better off at least understanding how it works and adding a comment.

my blind guess is that it's due to the fact that we also update a shipment (addToReceipts) and shipment already calls the refresh, so it's probably done to avoid double refresh.

it looks like the old logic set this to shipmentInstance?.recipient.

I think your way makes more sense but I just want to flag the behaviour change. Can this be set manually anywhere? I don't see a field for it in the mockups

I think the part of the code you found is deprecated and was used in the old shipment view page. It's called by ShipmentController.receiveShipment.

The "new" (becoming the old one 😆 ) partialReceiving does this:

"new" old 🙃 I can't wait to remove the old GSP pages so we can remove all this confusing duplicate logic

can you use SessionManager.getUser() instead? I wrote that last year as a replacement for authservice

@ewaterman SessionManager has a bug, I couldn't use it. I got the NonUniqueResultException given that User is taken from the http session or something like that and hibernate can't handle it when persisting the user (additional get would be needed)
I can see that in AuthService there must have been a similar problem:

 void setCurrentUser(User user) {
        if (!threadLocalUser) {
            threadLocalUser = new ThreadLocal<User>()
        }

        // misuse get() to prevent javax.persistence.EntityExistsException
        threadLocalUser.set(user?.id ? User.get(user.id) : null)
    }

    static User getCurrentUser() {
        return threadLocalUser?.get()
    }

// misuse get() to prevent javax.persistence.EntityExistsException

idk when this trend started but I don't like sub-packaging all our objects under /api. I'd rather it be grouped only by feature. I can somewhat understand why it is done for controllers (though even then I'd rather not because the *ApiController class name already explains that it is an Api Controller), but for all the files under /src (ie non-grails files) I'd rather everything go under the /org/pih/warehouse/receiving package

thoughts?

I think it will be very confusing to have both:

• org.pih.warehouse.receiving (old stuff)
• org.pih.warehouse.api.receiving.v2 (your new stuff)

I'd rather just have a single space for it all

you also need a check to see if the shipment is already fully received

do I? wouldn't the call from 29 answer also that? so that if there are any receipts in the shipment, that are PENDING this also means the shipment is not fully received?

I mean that you should not be able to create a receipt if the shipment has already been fully received. So you can't just check if there is a pending receipt because you might have a receipt that was fully submitted and contains the full quantities for the shipment.

I'm assuming you'd need to check the shipment status here to see if it is in [SHIPPED, PARTIALLY_RECEIVED] else error (we also don't want to be able to start a receipt if the shipment is still in the CREATED or PENDING status)

oh, ok, I understand now and fully agree.

@ewaterman fyi: checking PARTIALLY_RECEIVED is not necessary, because if it's partially received, this means there is a pending receipt and it's already handled above, so I didn't include the partially received.