OBPIH-6103 Create endpoint to create/edit product source details #4502

kchelstowski · 2024-02-09T14:52:22Z

No description provided.

kchelstowski · 2024-02-09T14:56:51Z

src/main/groovy/org/pih/warehouse/product/ProductSupplierDetailsCommand.groovy

+ productCode(nullable: true)
+ active(nullable: true)
+ }
+}


Since we are about to create 3 separate endpoints for now, I've decided to go with the command approach to make sure, that when using details endpoint, a user will potentially be able only to edit details part and that the user won't be able to include any other properties, e.g. throwing a list of productSupplierPreferences that would be persisted if I used the ProductSupplier domain as a command class.

cc @awalkowiak @alannadolny @drodzewicz @jmiranda

I wonder if we could somehow use shared constraints, like here: https://grails.github.io/legacy-grails-doc/3.3.0/ref/Constraints/Usage.html so we have a "single source of truth" regarding constraints, and won't have to change two places whenever constraints will change on the domain level.

@awalkowiak thanks, this feels interesting. I have not known this was possible. I will check it out and get back to you.

Yes, I agree with using shared constraints where possible/necessary. But we shouldn't have a command class unless there's a situation where the binding properties and constraints need to be different. Again, I don't want a layer of command classes for the sake of having command classes. The case that @kchelstowski brought up (i.e. limiting what data can be updated) is a very good question to ask when designing the REST API, but if we're exposing a large object graph in our GETs, then we should probably allow the user to post that object graph back as a POST/PUT to update the data. Otherwise, we should only expose those objects as their own resources or subresources.

Arbitrarily limiting the dependent objects that can be updated, when we're already exposing those objects in the parent resource seems confusing and possible a recipe for . With that said, @kchelstowski is absolutely right that we should be making that decision explicitly and not unnecessarily allowing the API client to reach deep into the object graph.

For example, I don't mind an API client updating a product supplier preference subresource from the product supplier resource (the example Kacper used above). However, I don't want to be allowing an API client to change a location or user associated with any of these resources or subresources within the object graph. That would be dangerous.

So we should test that case. If it's possible to update a username from a PUT request on the ProductSupplier resource.

And fwiw, the danger seems to exist whether I use a command or domain class since the data binding eventually occurs on the domain object. We can certainly limit the reach with a command class, but we can still do things we might not want to allow.

https://app.screencastify.com/v3/watch/PUdha5EEGFeyfH7OcFfh

If I can change product or supplier property of the command and change a user object deep in that graph, then it doesn't matter if we use a command or domain. We either need to be ok with this (and hope that our API users aren't malicious) or we need to explicitly protect against it by not binding objects to command objects and exposing them through the API. Or finding an alternative way to prevent deep binding like the video demonstrated.

The following article
https://spring.io/blog/2012/03/28/secure-data-binding-with-grails

is very old (Grails 2) but describes a few ways that we could implement this kind of protection from within the service

vendor.properties['vendorName'] = params // or... bindData(vendor, params, [include: ['vendorName']]) // or... bindData(vendor, params, [exclude: ['invoiceHelper']])

or domain

static constraints = { salary bindable: false }

I suspect Grails 3 @BindUsing and other data binding mechanisms might lead to a better approach, but I'll need to do some research.

I found an even worse issue than what @kchelstowski was worried about. And we are definitely susceptible to this given that none of our domain or command constraints are marked as final.

https://grails.org/blog/2022-09-28-static-binding-bug.html

@awalkowiak Regarding your comment about shared constaints, which class should be the single source of truth.

I'm guessing the domain in this case, and then we could override it if needed.

kchelstowski · 2024-02-09T14:58:35Z

grails-app/services/org/pih/warehouse/data/ProductSupplierService.groovy

+ throw new ObjectNotFoundException(productSupplierId, ProductSupplier.class.toString())
+ }
+ productSupplier.properties = command.properties
+ // TODO: To be replaced by a generic method created in OBPIH-6017 after merging into develop


as you can see, the if statement below is repeated here, and in the saveDetails method. I've implemented a method for that recently, while working on OBPIH-6017 (assignSourceCode): #4486
that we will be able to use here as soon as the feature branch is merged into develop (upgrade to grails 3.3.10) branch.

I'd like to keep the code below if possible i.e. revert the assignSourceCode. I like the idea of abstracting the generation and assignment, but I'm not yet comfortable with how it's done in #4486 so we can postpone that for now.

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

awalkowiak

I only have a question / discussion around constraints about how we want to move forward regarding sharing constraints between command and domain (see comment above)

…upplierDetailsCommand

awalkowiak

Overall LGTM, just one minor thing that I'd like to hear @jmiranda's opinion.

awalkowiak · 2024-02-13T16:34:22Z

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

@@ -28,4 +30,17 @@ class ProductSupplierApiController {
 productSupplierService.delete(params.id)
 render status: 204
 }
+
+ def create(ProductSupplierDetailsCommand productSupplierCommand) {


Is the naming productSupplierCommand here used on purpose (without Details)? If not I think it should match the command name

you are right, this is inconsistency from my side, so answering - it was not intentional.

awalkowiak · 2024-02-13T16:36:00Z

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

@@ -28,4 +30,17 @@ class ProductSupplierApiController {
 productSupplierService.delete(params.id)
 render status: 204
 }
+
+ def create(ProductSupplierDetailsCommand productSupplierCommand) {
+ ProductSupplier productSupplier = productSupplierService.saveDetails(productSupplierCommand)


I wonder if the word create would not be more suitable here instead of save, but I'll let @jmiranda decide what he prefers here. Other than that and one minor naming thing above it LGTM.

to include my opinion - I'd stick with save, since we are actually calling the db and saving the entity (and GORM's method is named save, not create, so to kinda follow the convention)
But I don't mind both, so let me know what you guys prefer.
I can also see that Grails' docs probably follow save/update pattern: https://docs.grails.org/latest/guide/REST.html

Looking into this docs, it also suggests calling the action save if you do POST to the /productSuppliers, so then if you'd like to follow it, then you need to change the action name to save.

Yeah, but in the controllers we used to use „create” as a method name, hence I followed the convention + I wanted the generic url handler to handle the action, which it wouldn’t if I used „save” as controller’s method name

Sounds good.

jmiranda · 2024-02-14T16:11:01Z

grails-app/services/org/pih/warehouse/data/ProductSupplierService.groovy

+ }
+
+ ProductSupplier saveDetails(ProductSupplierDetailsCommand command) {
+ validateProductSupplierDetails(command)


You can safely ignore this now. The previous comments include my suggestions on how to refactor these methods.

ProductSupplier productSupplier = new ProductSupplier(command.properties) if (command.hasErrors()) { productSupplier.errors = command.errors return productSupplier } // do some stuff if (!productSupplier.hasErrors()) { productSupplier = productSupplierGormService.save(productSupplier) } return productSupplier

should service throw exception

static vs compilestatic

@GrailsCompileStatic

@CompileStatic

@jmiranda To add something to here - I feel like we should focus on building a proper rest API, and stick to one approach or convention (in this case I think we should stick to throwing exception or error and handling it in error controller instead returning products supplier object with errors copied from command), and not mix into it different things to "satisfy" other potential use cases (calls from other services or non API controllers)

cc @kchelstowski, @drodzewicz @alannadolny

I agree we should stick to a single convention, but we haven't spent enough time to come up with that convention yet, so this PR is bringing up a lot of questions that haven't been answered yet. Honestly, I would prefer to keep building our API the way we have, using the BaseDomainApiController/GenericApiController until we have completed our research on REST API conventions or unless we need to extend the functionality of those controllers.

The main reason I brought up the idea of returning the domain object with errors idea is because 1) we've got a lot going on with the command object and domain object that is making things more unclear and 2) I have no idea what an out of the box Grails 3 project with the REST profile would do.

I would suspect that the service would need to do something like save(failOnError:true) in order to throw an exception and I assume the Grails developers would consider that non-standard.

With that said, I'm fine with throwing the ValidationException for now since the client (controller in this case) can handle the exception or let it propagate to the errors controller to convert to a proper error response.

jmiranda · 2024-02-15T00:15:53Z

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

+ ProductSupplier productSupplier = productSupplierService.saveDetails(productSupplierDetailsCommand)
+
+ response.status = HttpStatus.CREATED.value()
+ render([data: productSupplier.toJson()] as JSON)


Remove the toJson from render invocation in the create and update methods, and then add this to Bootstrap.groovy.

JSON.registerObjectMarshaller(ProductSupplier) { ProductSupplier productSupplier -> return productSupplier.toJson() }

You'll also need to add one for ProductSupplierPreference.

JSON.registerObjectMarshaller(ProductSupplierPreference) { ProductSupplierPreference productSupplierPreference -> return productSupplierPreference.toJson() }

And the supplier object in the ProductSupplier.toJson() is missing an ID, which initially prevented me from copying the JSON returned by a GET request and sending it in a PUT request.

Here's my attempt at the create endpoint. The validation exception should have a better message, but I hate that I need to pass a message there.

def create(ProductSupplierDetailsCommand command) { if (command.hasErrors()) { throw new ValidationException("validation errors", command.errors) } ProductSupplier productSupplier = productSupplierService.saveProductSupplier(params.id, command) response.status = HttpStatus.CREATED.value() render([data: productSupplier] as JSON) }

The reason I would prefer to use saveProductSupplier over saveDetails or save is because the ProductSupplierService will probably also house the code to save other objects like preferences and packages that are associated with a product supplier.

Is there a deeper reason why you prefer Bootstrap over toJson() here? I think recently we've discussed that we wanted to avoid using Bootstrap and stick to toJson() for now even if it's not the best approach possible.

The reason I went with saveDetails is for this method not to be confusing - doing it as saveProductSupplier in my opinion suggests, that we could update all product supplier's properties there - this is why I haven't named it like this.
But if you guys are ok with that, then I can change it.

Regarding your first comment, we should continue to use the JSON marshaller framework (including marshaller registration in BootStrap.groovy) until we have migrated to JSON views.

Regarding the second one, I understand your point. We just have a convention of using the name action, like saveProduct, saveProductSupplier.

We haven't really discussed having method names that imply scope. And I would say that the scope should be explicitly defined through better security measures, not though naming. But if we need

If we want to adopt a new naming convention we should create a proposal on the new naming convention and agree on it before applying it.

Here's an example of what a naming convention might look like. I don't like all of these convention, but I like the general idea of defining each convention, providing some justification and demonstration through a code example.
https://sergiodelamo.com/blog/grails-extra-conventions.html

jmiranda · 2024-02-15T00:31:10Z

grails-app/services/org/pih/warehouse/data/ProductSupplierService.groovy

@@ -355,4 +357,36 @@ class ProductSupplierService {
 void delete(String productSupplierId) {
 productSupplierGormService.delete(productSupplierId)
 }
+
+ private static void validateProductSupplierDetails(ProductSupplierDetailsCommand command) {


As I mentioned on the tech huddle, let's move the validation inside each method.

Also, I finally realized what was bothering we about the validation code. The command validation should happen in the controller. The domain validation should happen in the service.

jmiranda · 2024-02-15T00:32:59Z

grails-app/services/org/pih/warehouse/data/ProductSupplierService.groovy

+ }
+ }
+
+ ProductSupplier saveDetails(ProductSupplierDetailsCommand command) {


Let's just follow our naming convention and call this saveProductSupplier. We should have a single method called that figures out whether it's creating or updating (i.e presumably checking the ID) and will make the determination of whether it needs to generate the unique code. You could also remove the distinction between create and update, always generating the unique code if the code property is null.

Let's also change the command to ProductSupplierCommand or ProductSupplierDetails. I would prefer just binding to ProductSupplier in the controller, but if we decide that's too dangerous then let's make the command class as plain and simple as possible.

Having two extra specifies "Details" and "Command" just adds slight confusion for me. I think if we were dealing with a case where we needed multiple command classes it might make sense to expand to include multiple classes (e.g. ProductSupplierDetails, ProductSupplierSummary). But I suspect we're going to have separate command classes for the parent resource and subresources, not multiple command classes for the parent resource. In other words, the next class we're going to create will likely be something ProductSupplierPreferences or we'll extend ProductSupplierDetails to include these other associations.

Aside: I've used Command as the convention for command classes in the past because without it I was worried about creating classes that looked similar to domains, but didn't behave like them. However, I don't love using Command as the suffix so I'd be happy with figuring out a convention like "Details" or "Info" as the suffix, if there's a hierarchy of classes that we can create to deal with everything we need to. Otherwise, using "Command" as the convention is fine for now. We can always refactor that later when we give it a bit more thought.

Caveat: I understand what @kchelstowski is saying about using the command class to limit the data that can be bound, and therefore updated, through the API. So we should definitely be mindful of that.

Here's my attempt at the generic save (used for create or update).

ProductSupplier saveProductSupplier(String id, ProductSupplierCommand command) { ProductSupplier productSupplier if (id == null) { productSupplier = new ProductSupplier(command.properties) } else { productSupplier = productSupplierGormService.get(id) if (!productSupplier) { throw new ObjectNotFoundException(id, ProductSupplier.class.toString()) } productSupplier.properties = command.properties } if (!productSupplier.code) { productSupplier.code = identifierService.generateProductSupplierIdentifier(command?.product?.productCode, command?.supplier?.code) } return productSupplierGormService.save(productSupplier) }

Note: I am not handling the validation on the product supplier domain object. If there's a desire to throw a validationexception from the service, you'll need to add some code here to handle it.

@awalkowiak @kchelstowski I'll let you decide how you want to handle this

keep separate save and update methods

use a single save method

jmiranda · 2024-02-15T01:06:24Z

grails-app/services/org/pih/warehouse/data/ProductSupplierService.groovy

+ return productSupplierGormService.save(productSupplier)
+ }
+
+ ProductSupplier updateDetails(ProductSupplierDetailsCommand command, String productSupplierId) {


The ID property should probably be bound as part of the command class or manually set as a property in the controller action. This is one of the reasons I don't like adding a layer of command objects. We now have to covert back and forth, and it's not as straightforward as just using the domain object.

jmiranda · 2024-02-15T04:44:06Z

src/main/groovy/org/pih/warehouse/product/ProductSupplierDetailsCommand.groovy

+ productCode(nullable: true)
+ active(nullable: true)
+ }
+}


Yes, I agree with using shared constraints where possible/necessary. But we shouldn't have a command class unless there's a situation where the binding properties and constraints need to be different. Again, I don't want a layer of command classes for the sake of having command classes. The case that @kchelstowski brought up (i.e. limiting what data can be updated) is a very good question to ask when designing the REST API, but if we're exposing a large object graph in our GETs, then we should probably allow the user to post that object graph back as a POST/PUT to update the data. Otherwise, we should only expose those objects as their own resources or subresources.

jmiranda · 2024-02-15T05:50:39Z

src/main/groovy/org/pih/warehouse/product/ProductSupplierDetailsCommand.groovy

+ productCode(nullable: true)
+ active(nullable: true)
+ }
+}


And fwiw, the danger seems to exist whether I use a command or domain class since the data binding eventually occurs on the domain object. We can certainly limit the reach with a command class, but we can still do things we might not want to allow.

https://app.screencastify.com/v3/watch/PUdha5EEGFeyfH7OcFfh

If I can change product or supplier property of the command and change a user object deep in that graph, then it doesn't matter if we use a command or domain. We either need to be ok with this (and hope that our API users aren't malicious) or we need to explicitly protect against it by not binding objects to command objects and exposing them through the API. Or finding an alternative way to prevent deep binding like the video demonstrated.

jmiranda · 2024-02-15T05:59:36Z

src/main/groovy/org/pih/warehouse/product/ProductSupplierDetailsCommand.groovy

+ productCode(nullable: true)
+ active(nullable: true)
+ }
+}


The following article
https://spring.io/blog/2012/03/28/secure-data-binding-with-grails

is very old (Grails 2) but describes a few ways that we could implement this kind of protection from within the service

vendor.properties['vendorName'] = params // or... bindData(vendor, params, [include: ['vendorName']]) // or... bindData(vendor, params, [exclude: ['invoiceHelper']])

or domain

static constraints = { salary bindable: false }

I suspect Grails 3 @BindUsing and other data binding mechanisms might lead to a better approach, but I'll need to do some research.

jmiranda · 2024-02-15T06:03:41Z

src/main/groovy/org/pih/warehouse/product/ProductSupplierDetailsCommand.groovy

+ productCode(nullable: true)
+ active(nullable: true)
+ }
+}


I found an even worse issue than what @kchelstowski was worried about. And we are definitely susceptible to this given that none of our domain or command constraints are marked as final.

https://grails.org/blog/2022-09-28-static-binding-bug.html

jmiranda · 2024-02-15T06:16:31Z

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

+ }
+
+ def update(ProductSupplierDetailsCommand productSupplierDetailsCommand) {
+ ProductSupplier updatedProductSupplier = productSupplierService.updateDetails(productSupplierDetailsCommand, params.id)


And here's the update endpoint.

def update(ProductSupplierCommand command) { if (command.hasErrors()) { throw new ValidationException("validation errors", command.errors) } ProductSupplier productSupplier = productSupplierService.saveProductSupplier(params.id, command) render([data: productSupplier] as JSON) }

I still don't like the params.id as an argument to this method, but understand why it was done that way.

Regarding the response for create and update, it is possible that we return the object as-is, but I am still working out the correct "response" for these endpoints based on best practices. There are a lot of good arguments for both sides.

jmiranda · 2024-02-15T06:17:17Z

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

+ def update(ProductSupplierDetailsCommand productSupplierDetailsCommand) {
+ ProductSupplier updatedProductSupplier = productSupplierService.updateDetails(productSupplierDetailsCommand, params.id)
+
+ render([data: updatedProductSupplier.toJson()] as JSON)


remove toJson

kchelstowski · 2024-02-15T10:18:45Z

@jmiranda

And fwiw, the danger seems to exist whether I use a command or domain class since the data binding eventually occurs on the domain object. We can certainly limit the reach with a command class, but we can still do things we might not want to allow.

Yeah, this is why I've mentioned many times that I am not fan of relying on domain classes for binding or inside a command class, because such "weirds" dangerous things could happen if we rely on domain classes, not on POJOs which we have control of most often with third party libraries like MapStruct which are very flexible and configurable for any use case.

So I am not surprised that we can edit the Product and Organization, because those are existing domain object that are bound to the transaction/session and any changes on them would be tracked.
An easy way to prevent from updating them would be to either create some generic method, or just write those two lines manually after binding to the product suppleier domain:

productSupplier.properties = command.properties
productSupplier.product?.discard()
productSupplier.supplier?.discard()

and none changes would be persisted to product and supplier 🙂

I wouldn't agree that this makes no difference between using the command class and domain class for binding, because what I meant by "updating some additional properties" I meant rather those, that are not meant to be touched during the saving details process, e.g. binding productSupplierPreferences, productPackages, which would be possible if we used domain class, and is not possible with the command I've created.

jmiranda · 2024-02-15T16:19:07Z

@kchelstowski @awalkowiak i'll try to go through each discussion and determine whether it should be dealt with now or later.

Edit: I've added a 👍 to comments that contain advice we should tackle now and a 🚀 for things we can punt until later.

jmiranda · 2024-02-15T16:49:39Z

An easy way to prevent from updating them would be to either create some generic method, or just write those two lines manually after binding to the product suppleier domain:

productSupplier.properties = command.properties
productSupplier.product?.discard()
productSupplier.supplier?.discard()

I understand your point, but this is not a sustainable approach. You need to do this everywhere you're using the "productSupplier.properties = ... " data binding mechanism. And if you add a new association to ProductSupplier, you need to remember to go back and discard the association.

We either need to make this "discard" mechanism more automatic (i.e. annotation on the service method?) OR better yet we need the protection to happen during data binding phase in the controller. Either a feature within the @BindUsing annotation or explicit whitelisting properties (i.e. bindData(object, includes: [])) the client is allowed to update for a given service method. Actually, that mechanism could exist at the controller level

bindData(command, params, [include: [...]])

or the service level

bindData(productSupplier, command.properties, [include: ['id', 'code', 'name', ...]])

We could potentially make this more palatable and DRY by create a command class interface to requires us to expose the properties to include and exclude via the command class.

bindData(productSupplier, command.properties, [include: command.includeProperties, exclude: command.excludeProperties])

So I am not surprised that we can edit the Product and Organization, because those are existing domain object that are bound to the transaction/session and any changes on them would be tracked.

The point was that if you're binding objects to a domain or command, you can edit the properties of any object in their graph linked to that object. In order for command objects to be used to prevent this then your command object needs to bind only primitives and Strings (i.e. the PK of the object rather than the object itself).

String productId
String supplierId

Which, in my mind, defeats the purpose of using a command object for data binding. So again, there needs to be another way to protect against deep data binding.

// This is a hypothetical for what i'm looking for
@BindUsing(onlyBindPrimaryKey=true)
Product product

However, I'm actually wondering whether the use of primitives + String in the command class would actually be all we need.

productSupplier.properties = command.properties

It would be interesting to see if this approach yields what we're looking to do. It doesn't provide all of the validation we need at the command object level (i.e. we can have a productId in the command, but it might not actually map to a Product in the database), but that would be caught when we try to validate and save the domain.

Anyway, that would be fine with me for now.

kchelstowski · 2024-02-15T17:33:37Z

@jmiranda I throw one more possible option to avoid updating nested properties, e.g. we can just define a proper validator in the command's constraints and see if a property (let's say - product) is dirty:

 static constraints = {
      ...
      product(validator: { Product product ->
          if (product?.dirty) {
              return ['validator.product.cantBeDirty']
          }
          return true
      })
  }

By doing so, if a user tries to update a product via product supplier endpoint, he/she would get a validation exception.

I've also tried binding the Product via @BindUsing like:

@BindUsing({ obj, source -> Product.read(source["id"])})
Product product

but had no success in that.

I know the solution with constraints is not ideal, but still gets us closer than with the discard(), because at least it needs to be done in one place in order to work fine.

jmiranda · 2024-02-15T17:53:40Z

Ooh dang. That's interesting. It feels a bit heavy-handed (a slap instead of an ignore) and it requires maintainance for whenever we add new associations, but I think it's an interesting avenue to explore, especially if we can make it a little more reusable.

ProductSupplierCommand.groovy

static constraints = { 
    product: shared: 'myFavoriteLittleDirtyConstraint'
    supplier: shared: 'myFavoriteLittleDirtyConstraint'
}

runtime.groovy

grails.gorm.default.constraints = {
  myFavoriteLittleDirtyConstraint = { value, object ->
     if (value?.dirty) {
         // TODO exercise for reader: return a specific error for each class (if it exists), generic error message if not
         return ['validator.myFavoriteLittleDirtyConstraint.message']
     }
     return true
  }
}

but had no success in that.

Can you clarify what you meant by that? The product wasn't bound at all or wasn't read-only and still bound properties we didn't want? Remember the "read" doesn't make the object read-only as-in unmodifiable, it just disables the dirty detection and presumably automatic flush at session end.

Source: https://docs.grails.org/latest/ref/Domain%20Classes/read.html

However, this is the path I'd like to explore a little more. I'm guessing we can do some interesting things with the BindUsing annotations.

@BindUsing({ obj, source -> Product.read(source["id"])})
Product product

If not, we can explore creating our own annotation or extend the existing one to do what we want i.e. discard object.

kchelstowski · 2024-02-15T17:55:22Z

Can you clarify what you meant by that? The product wasn't bound at all or wasn't read-only and still bound properties we didn't want?

Was not bound at all

kchelstowski · 2024-02-19T09:04:18Z

@awalkowiak @jmiranda I've addressed suggestions marked with " 👍 "

awalkowiak

I think all of the change requests were resolved. LGTM

* OBPIH-6103 Create endpoint to create/edit product source details * OBPIH-6103 fix conflicts * OBPIH-6103 Import constraints from ProductSupplier domain to ProductSupplierDetailsCommand * OBPIH-6103 Change the name of the argument to include Details suffix * OBPIH-6103 Fixes after review

kchelstowski requested review from jmiranda, awalkowiak, drodzewicz and alannadolny February 9, 2024 14:52

kchelstowski self-assigned this Feb 9, 2024

OBPIH-6103 Create endpoint to create/edit product source details

537929e

kchelstowski force-pushed the OBPIH-6103 branch from 84a7042 to b03cadb Compare February 9, 2024 14:54

OBPIH-6103 fix conflicts

a8c2a9b

kchelstowski force-pushed the OBPIH-6103 branch from b03cadb to a8c2a9b Compare February 9, 2024 14:54

kchelstowski commented Feb 9, 2024

View reviewed changes

alannadolny approved these changes Feb 12, 2024

View reviewed changes

drodzewicz reviewed Feb 12, 2024

View reviewed changes

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

drodzewicz approved these changes Feb 12, 2024

View reviewed changes

awalkowiak reviewed Feb 13, 2024

View reviewed changes

OBPIH-6103 Import constraints from ProductSupplier domain to ProductS…

160d57b

…upplierDetailsCommand

kchelstowski requested a review from awalkowiak February 13, 2024 14:16

awalkowiak reviewed Feb 13, 2024

View reviewed changes

OBPIH-6103 Change the name of the argument to include Details suffix

a3bcd78

jmiranda reviewed Feb 14, 2024

View reviewed changes

jmiranda requested changes Feb 15, 2024

View reviewed changes

OBPIH-6103 Fixes after review

927465f

kchelstowski requested a review from awalkowiak February 19, 2024 09:03

awalkowiak approved these changes Feb 19, 2024

View reviewed changes

awalkowiak merged commit 9102625 into feature/product-supplier-list-redesign Feb 19, 2024

awalkowiak deleted the OBPIH-6103 branch February 19, 2024 11:25

OBPIH-6103 Create endpoint to create/edit product source details #4502

OBPIH-6103 Create endpoint to create/edit product source details #4502

Conversation

kchelstowski commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awalkowiak left a comment • edited Loading

Choose a reason for hiding this comment

awalkowiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmiranda Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchelstowski commented Feb 15, 2024

jmiranda commented Feb 15, 2024 • edited Loading

jmiranda commented Feb 15, 2024 • edited Loading

kchelstowski commented Feb 15, 2024 • edited Loading

jmiranda commented Feb 15, 2024

kchelstowski commented Feb 15, 2024

kchelstowski commented Feb 19, 2024

awalkowiak left a comment

Choose a reason for hiding this comment

awalkowiak left a comment •

edited

Loading

jmiranda Feb 14, 2024 •

edited

Loading

jmiranda commented Feb 15, 2024 •

edited

Loading

jmiranda commented Feb 15, 2024 •

edited

Loading

kchelstowski commented Feb 15, 2024 •

edited

Loading