why we first import the template and then check if the errors are not empty?

some of the requirements for import are

if I enter a stock record that does not exist in inventory, then the pick isn’t imported for these incorrect lines, and all valid lines show the pick from the import successfully and the invalid lines remain empty and are highlighted in red to complete the pick

which means that user expect to import valid lines and get errors for invalid ones.

The way Manon explained this to me is that user might import the file, export pick file, fix rows that were invalid and were not imported and then import the file again.

Basically main point is that we don't want to fail the import if there are invalid lines.
If you guys disagree with that, then this needs to be discussed with Manon and Kelsey.

Maybe from a user perspective it does have a sense, but from a best practices/REST/anything perspective, this smells to me.

Moreover, prove me wrong, but in case of situation when a few items have errors and user would want to fix them, I assume the user wouldn't remove the valid ones (those that are already persisted) from the sheet.
Would we actually update an existing item or we would create a new pick list item?
Looking quickly at the code, I'm concerned that we actually wouldn't update the existing item.

Sorry, for some reason I'm not seeing all comments when I click on Files Changed tab so I missed this one the first time through.

Maybe from a user perspective it does have a sense, but from a best practices/REST/anything perspective, this smells to me.

I know we've talked about this a bit already but just wanted to provide some context in case someone stumbles upon this conversation and gets confused.

Remember, this isn't supposed to be a REST API. This is a multi-part request that was implemented as a REST endpoint. Not to say it couldn't be implemented as a REST endpoint (see the proposals discussed earlier
https://pihemr.atlassian.net/wiki/spaces/OB/pages/3075342378/Data+Imports), but we're simply uploading a file and processing the file on the server as we would an XLS or CSV file. Imagine this happening from an HTML form.

The difference between what we're doing here and uploading a form via a GSP is that we usually don't have React interacting with the server via standard HTTP requests. And perhaps we shouldn't ... but until we create a convention around importing data via the API, it's probably safe to go with the HTTP approach.
https://pihemr.atlassian.net/wiki/spaces/OB/pages/3075342378/Data+Imports

So our convention with uploaded data files (i.e. Configuration > Import Data) has been to prevent data from being saved unless all the data is valid. But the case that Manon wants to implement here is slightly different from most of those because the picklist item data we're importing are essentially "edits" to the existing picklist. That's not entirely true since the user will be importing brand new picklist items after clearing the picklist.

The confusion here might be due to the fact that we're exposing this endpoint to a React client where the expectation is to handle a multi-part request and return a JSON response i.e. we have to return the errors as JSON. I don't know if there's a better way for the server to deal with this type of interaction. But since Axios is an HTTP client (not a REST) client it seems reasonable to implement it this way.

Last point. You're correct that this might be handled differently if it was a REST endpoint. But there's precedence for implementing this type of "save what's valid, return errors for what's not" using the 207 Multi-Status status code. I have a bunch of resources saved that describe this when implementing "bulk api" behavior. Here are a few:

• https://tyk.io/blog/api-design-guidance-bulk-and-batch-import/
• https://stackoverflow.com/questions/28596688/rest-api-bulk-create-or-update-in-single-request

Moreover, prove me wrong, but in case of situation when a few items have errors and user would want to fix them, I assume the user wouldn't remove the valid ones (those that are already persisted) from the sheet.

This is a good point. But my understanding is that uploading the file multiple times would not create duplicate picklist items (which I think is your concern). It would just add new lines and update lines that have changed. Furthermore, removing lines from the import file might actually signify that we want to remove them. But that is not a requirement here, as far as I know, so just ignore that point.

In either case, this would be a good test case to implement in an integration test.

• User clears picklist
• User uploads a data file with 10 lines, 8 valid, 2 invalid
• System creates 8 picklist items
• User fixes 2 invalid lines
• User uploads a data file with 10 lines
• System ignores 8 valid lines, creates 2 picklist items

We could even add in a listener on Hibernate events to make sure we're not touching the 8 valid lines on the second import.

😲 I guess this answers my first comment, but I'm in shock if we really want to go this path.
This smells like an anti-pattern to me, that even though there are errors, we would get 200 status and act like everything went well.
P.S. I know this was not your choice to do it this way, but wanted to raise my astonishment.

side comment would be it we want to have those messages translated.

I guess this answers my first comment, but I'm in shock if we really want to go this path.
This smells like an anti-pattern to me, that even though there are errors, we would get 200 status and act like everything went well.
P.S. I know this was not your choice to do it this way, but wanted to raise my astonishment.

@kchelstowski Can you elaborate on this? I feel like I'm missing some context here.

@jmiranda this comment was about importing the template with e.g. 50 valid lines and 50 invalid - I was just surprised that we would proceed even if we were about to save 50 items out of 100.
This is why I was surprised that we first try to import the items and then eventually check if the errors are not empty - in typical scenario we do the opposite.

I'm wondering why we don't return the imported items in the response

This is the pattern that we have followed in most of the places where we update some data in the table and folow up with a GET request.

We could talk about it more and have a concrete pattern to follow for future implementations.

I think it's safer to refretch the pickPageItems through the normal mechanism. That way the endpoint here can be free of a potential performance problem if the picklist is large. Let the client fetch deal with the usability problem of a slow load.

This method basically explains why I hate reviewing and reading those.
I would really love to have this kind of validation happening in a proper validator of a command object. This would reduce the amount of complexed code in one method, or at least make it more readable if we had this type of validation in the constraints of the command object, e.g.

static constraints = {
  id(blank: false)
}

with a properly constructed error message instead of

 if (!params.id) {
   params.errors.push("Requisition item id is required")
}
...

etc..

As a "proof of concept" I would suggest looking at ProductSupplierAttributeCommand/ProductSupplierAttributeBatchCommand where I implemented a bunch of complexed validation along with the updateAttributes method where I handled a similar case to your with appending errors to the array.

P.S. I assume you would not want to change it now, but it would be good to copy this comment to the idea garden ticket you were involved in.

I would really love to have this kind of validation happening in a proper validator of a command

@kchelstowski I agree. In this particular case, the command object would need to be pretty smart to handle the logic. But if we can make it work, let's do it. And as you suggested in the past, this might be a good use case for MapStruct, since we need to convert between different beans/domains. I'm not sure we want to do a proof of concept of all of that in this PR, but it seems like it would be a good proving ground for some future scope.

@kchelstowski Can you create a discovery ticket for this particular refactoring? Just provide a link to the offending code with a few bullets suggesting what we could / should investigate.

I can see that in this method there is the same issue that I've recently fixed. Could you please add in this method the association between requisition and the picklist?
requisition.picklist = picklist

That was only necessary because we were rendering the response with an invalid/unpersisted object so I don't think we're going to see that issue here.

Can we just pass in the picklist items, not the pick page items? Or does that method not exist? If it doesn't we should create it and then start to refactor other methods to use the proper API.

FWIW I don't think this method should exist. It doesn't make sense from an API perspective. It feels like we're packing the required data into unnecessary proxy objects.

void createOrUpdatePicklistItem(StockMovement stockMovement, List<PickPageItem> pickPageItems) 

Something like this would be better

void createOrUpdatePicklistItems(StockMovement stockMovement, List<PicklistItem> picklistItems) 

This would be even better

void createOrUpdatePicklistItems(Picklist picklist, List<PicklistItem> picklistItems) 

But we're also in a situation where it probably makes sense to do the persistence in the import method.

void importPicklistItems(List<Map> data, StockMovement stockMovement, 
    List<PickPageItem> pickPageItems) {

    ...
    Picklist picklist = stockMovement.requisition.picklist
    data.each { row ->
        PicklistItem picklistItem = new PicklistItem(row...)
        picklist.addToPicklistItems(picklistItem)
    }
    picklist.save()
}

If we were about to leave it in a separate method, I would suggest calling the .save() on a picklist item (not persisted object) instead of on the picklist and saving the picklistItems via cascade saving.
The picklist is already known to Hibernate, and any of the changes to the picklist (e.g. adding the items to the picklistItems list) would be handled by Hibernate at the transaction commit without having to call .save() explicitly.
The reason I suggest that is that saving the child objects (especially when there are a lot of them) via cascade of the parent (picklist) might cause huge performance issues, difficult to detect - once in another project (Java+Spring) there was a huge performance issue when persisting some complexed stuff due to the cascade saving, and when I changed it to save children separately, the time got reduced from about 4 minutes to 2 seconds....

That performance issue sounds like maybe something else was happening though. But I take your point which seems to be "let's do things explicitly" vs "relying on Hibernate magic" 🧙‍♀️

But in my mind, the main reasons to use Hibernate is to

• manage your associations (cascades)
• avoid costly saving (worse-case: flushing) each individual child object on its own.
• provide a mechanism for rolling back a transaction

This approach is ignoring the "manage association" benefit of Hibernate, which could lead to performance degredation due to the many flushes that could be happening and might not allow you to rollback the flushed data if an exception happens afterwards.

data.each { 
    PicklistItem picklistItem = new PicklistItem(it)
    picklistItem.picklist = picklist
    picklistItem.save(flush:true)
}

This approach let's Hibernate do its thing (based on cascading rules defined on the domain) and would rollback the picklist item data if there was a validation error in picklist or somewhere else.

data.each { 
    PicklistItem picklistItem = new PicklistItem(it)
    picklist.addToPicklistItems(picklistItem)
}
picklist.save(flush:true)

This is all theoretical though. You seem to distrust Hibernate and I think that's a healthy attitude to have with respect to things that perform magic.

So it would be a good discovery ticket to write this in multiple ways and then perform some integration tests to see which is the right approach. Then we can create a convention around it.

• performance with a large number of picklist items
• can you rollback the data at different points in the Hiberate session/transaction
  • my hunch is that both would be rollback-able even if data has been saved with flush:true
• if we were to use different cascade behaviors (https://docs.grails.org/6.1.2/ref/Database%20Mapping/cascade.html), would either of these perform in a way that is not expected

One thing to note in our case is that whenever we save a PicklistItem we're executing the "refresh product availability" code. So that adds a separate wrinkle. Do we want all of those background tasks to happen all at once or when we save the picklist at the end? In the former case we can use the disableRefresh mechanism so it should be a moot point.

@jmiranda as for this one, I might be overcautious - I've relied on the cascade many times before and have not experienced any issues, but I also remember one situation (described above) that was due to the cascade saving.
I wouldn't say I want to do things explicitly, because I love Hibernate, and we should rely on it in most of the cases, e.g. I wouldn't say we should write:

Picklist picklist = Picklist.get(...)
PicklistItem picklistItem = new PicklistItem(...)
picklistItem.picklist = picklist
picklist.picklistItems.add(picklistItem)
picklistItem.save()

instead of:

Picklist picklist = Picklist.get(...)
PicklistItem picklistItem = new PicklistItem(...)
picklist.addToPicklistItems(picklistItem)
picklistItem.save()

because this would be an overkill. The recent bug we experienced around requisition/picklist was in 1:1 association where this have to be handled manually (because Hibernate adds it AFTER the session is closed, this is why it caused issues, whereas the addTo* (e.g. addToPicklistItems adds the association in two objects immediately))

An important note is that we hardly ever need to use flush: true while saving - I'd say the flush: true would be needed if for example constraints of the picklistItem rely on already existing picklistItems saved in the same transaction, to validate them properly.

In case of:

data.each { 
  Picklist picklist = Picklist.get(...)
  PicklistItem picklistItem = new PicklistItem(...)
  picklist.addToPicklistItems(picklistItem)
  picklistItem.save()
}

Hibernate just creates a queue and triggers all the saves at the transaction commit.

So TL;DR would be - I trust in Hibernate magic and we should rely on it in most of the cases, but at the same time we need to know what happens behind the scenes and what might be the edge cases around some of the operations.
As for the performance issues around the cascade - I would skip it for now and check such things in a discovery/spike ticket around Hibernate - including the impact of the batchSize property, etc.

This discussion has been moved to a discovery ticket.
https://pihemr.atlassian.net/browse/OBIG-8

@drodzewicz You can ignore this discussion.

I'm not sure if this additional request after the delete is required here, since this should always be an empty picklist at this point.

As a side note, I also think that the name of that method might be misleading - we are in clearPicklist method and suddenly we trigger a method with "import" in the name.

I don't mind doing it explicitly, but nullable: false is the default setting, so we could get rid of the id here.

• you've added the Validateable, and those constraints, but I can't see you eventually validating the command.
  validating is a wrong word, because it's validated while being bound, but I can't see you eventually returning the errors.
  We could skip it for now, since previously there was not such check for the params, but just wanted to emphasize, that currently the errors would be "swallowed".

This is what I was expecting. Thank you.

The point I was trying to make about moving this to a non-API controller was that we don't necessarily need this feature to be exposed by our API.

The request is going to be multipart file so we could just have an importCsv or importXls action in the PicklistController like we do with the StockMovementController.

Note: We've added these endpoints to the PartialReceivingApiController and LocationApiController and I would argue that it was probably the wrong place to add them.

def importCsv(ImportDataCommand command) { 
    // Lookup the picklist
    Picklist picklist = picklistService.getPicklist(params.id)
    // ... and then do sany validation on what we have some far
    //  - picklist exists, otherwise throw exception 
    //  - any request parameters that are needed
    //  - data format of the input file, maybe the separator used
    
    // Or move as much of the validation into the command class validate() method if possible
    if (!command.validate()) {
         throw new ValidateException(command.errors)
    } 
    
    // You could also break the process down into the 
    if (!picklistImportService.validatePicklistCsv(command)) {     
        throw new ValidateException(command.errors)
    }
     
    picklistImportService.importPicklistItems(picklist, command.picklistItems)
}

PicklistImportService

// If you'd prefer to return a list of maps or DTOs that's fine too.
void buildPicklistItems(ImportDataCommand command) { 
    String csv = new String(command.importFile.bytes)
    char separatorChar = CSVUtils.getSeparator(csv, StockMovement.buildCsvRow()?.keySet()?.size())
    def settings = [separatorChar: separatorChar, skipLines: 1]
    Integer sortOrder = 0
    csv.toCsvReader(settings).eachLine { tokens ->
        def picklistItem = buildPicklistItem(tokens)       
        command.data.add(picklistItem)
    }
}

// Not sure if it's cool to do side effects to the command object in a validate method, 
// so if it makes more sense you can return the command object instead of a boolean 
boolean validatePicklistCsv(ImportDataCommand command) {
    // we might need custom logic to check against available items 
    // but i'm not going to be able to put any thought into how to handle that
    
    // validate each of the picklist items
    command.data { row ->  
        if (!row.validate() || row.hasErrors()) {
            command.errors << row.errors
        }
    }
    return command.hasErrors()
}


void importPicklistCsv(Picklist picklist, List<PicklistItem> picklistItems) { 
}

void importPicklistCsv(ImportDataCommand command) {
    Picklist picklist = Picklist.get(command.id)
 
    // Assuming we use DTOs for the CSV -> picklist item object 
    // but I'd also be ok if the buildPicklistItem() method returned
    // domain objects
    if (!command.validate() || !validatePicklistCsv(command)) {
        command.data.each { picklistItemDto ->
            PicklistItem picklistItem = picklistItemDto.toPicklistItem() 
            picklist.addToPicklistImtems(picklistItem)
        }
        // There's probably some further validation and error checking 
        picklist.save()
    }     
}

You do not need to go in this direction, I just wanted to show what I was expecting.

I moved the conversation about Data Import approaches to Confluence for further discussion.
https://pihemr.atlassian.net/wiki/spaces/OB/pages/3075342378/Data+Imports

I would really love to have this kind of validation happening in a proper validator of a command

@kchelstowski I agree. In this particular case, the command object would need to be pretty smart to handle the logic. But if we can make it work, let's do it. And as you suggested in the past, this might be a good use case for MapStruct, since we need to convert between different beans/domains. I'm not sure we want to do a proof of concept of all of that in this PR, but it seems like it would be a good proving ground for some future scope.

That was only necessary because we were rendering the response with an invalid/unpersisted object so I don't think we're going to see that issue here.

Since we're seeing this in a few places, let's go ahead and move this into the PickPageItem bean.

AvailableItem availableItem = 
    pickPageItem.getAvailableItem(params.binLocation, params.lotNumber)

PickPageItem

AvailableItem getAvailableItem(Location binLocation, String lotNumber) {
    return availableItems?.find { item ->
                Boolean binLocationMatches = params.binLocation ? item.binLocation?.name == params.binLocation : !item.binLocation
                Boolean lotMatches = params.lot == item.inventoryItem?.lotNumber
                binLocationMatches && lotMatches
}

One question ... is there any way that we could have multiple available items matching on lot number and bin location. My gut says no, but just wanted to make sure.

we shouln't. I dont think it is possible to create two stock records with same lot and binLocation

This is nit-picky but let's make the following changes:

• change the method name since we're not importing a template, we're importing picklist items from a template
• we don't really need StockMovement, so let's try to pass only the args that are needed to the method
• make the List data the last argument
• let's change the name of the data argument be more clear
• consider converting from Map to a Command class, if desired

void importPicklistItems(Picklist picklist, List<PickPageItem> pickPageItems, 
    List<Map> picklistItems) 

Passing pickPageItems here is bothering me but I don't see a good solution at the moment. Maybe if it looked like this (with pickPageItems being included with the stock movement)

void importPicklistItems(StockMovement stockMovement, List<Map> picklistItems)

... then it'd look cleaner. But I don't like the idea of passing a huge command object like StockMovement to a method that only needs access to a few objects under it.

It's possible that we could move all of this into a DTO/Command class hierarchy and that would have only what is necessary to do the work

void importPicklistItems(ImportPicklistCommand command) { 
    ...
}

class ImportPicklistCommand { 
    Picklist picklist
    Location currentLocation
    StockMovement stockMovement
    List<PicklistItemCommand> picklistItems
}

class ImportPicklistItemCommand {
    RequisitionItem requisitionItem
    List<AvailableItem> availableItems
    // Include this if preferred, but the command should only reference the objects it 
    // needed to perform validation and make decisions
    //PickPageItem pickPageItem

    // move validation logic here
}

Can we just bottle up the picklist items in their own list and pass them to the createOrUpdatePicklistItems() method? Even if that means creating a new method?

Can we just pass in the picklist items, not the pick page items? Or does that method not exist? If it doesn't we should create it and then start to refactor other methods to use the proper API.

FWIW I don't think this method should exist. It doesn't make sense from an API perspective. It feels like we're packing the required data into unnecessary proxy objects.

void createOrUpdatePicklistItem(StockMovement stockMovement, List<PickPageItem> pickPageItems) 

Something like this would be better

void createOrUpdatePicklistItems(StockMovement stockMovement, List<PicklistItem> picklistItems) 

This would be even better

void createOrUpdatePicklistItems(Picklist picklist, List<PicklistItem> picklistItems) 

But we're also in a situation where it probably makes sense to do the persistence in the import method.

void importPicklistItems(List<Map> data, StockMovement stockMovement, 
    List<PickPageItem> pickPageItems) {

    ...
    Picklist picklist = stockMovement.requisition.picklist
    data.each { row ->
        PicklistItem picklistItem = new PicklistItem(row...)
        picklist.addToPicklistItems(picklistItem)
    }
    picklist.save()
}

I don't feel good about chaning this method in this way.

Can you explain what this is doing and why it's necessary?
Is it possible that we could leave getPickPageItems() alone since it's being used elsewhere, but then transform those into whatever you're trying to do here.

List pickPageItems = getPickPageItems(stockMovementItem)
List pickPageItemsUltra = pickPageItems.collect { transformToSomethingUltra(it) }

Also, I would like to find another name for the createNewPicklistItems argument. It's possible I just don't understand what's happening here and it will become more intuitive once I understand the reason.

The main issue here was that getPickPageItems creates new picklist for items that don't have the picklist.

What I was trying to achieve is to specify to the method that I don't want to create a new picklist for items that don't have it, all I want is to get the existing data.

Let's call this ALLOW_OVERPICK.

I think if you pass an additional argument (stepNumber) to getStockMovement(id, stepNumber) you should get the pickPageItems populated, no?

well... not really
To be honest the method for getStockmovement that receives stepNumber is a little weird
It calls the getRequisitionBasedStockMovement with stepNumber but looking into this function stepNumber isn't being used.

I can't really make any sense of git logs on why this argument was added for getRequisitionBasedStockMovement

Yeah, that's weird. I just remembered that handle the GET stock movement items request via a separate API. So maybe we used to populate the stock movement items (pick page items, etc) from within that method, but needed to pull it out into a separate API endpoint when we added pagination support for the stock movement items API.

🤷

I don't remember why we these arguments are Strings, but I would be careful with unintended consequences if you haven't changed all String stepNumber variables to Integer throughout the code (lines 733-739).

With that said, I'm fine with it as long as we make the change throughout or as long as there some sort of implicit coercion happening that allows for the case statements and comparisons to essentially treat the following as true.

assert 1 == "1"
assert "1".equals(1)

If not, we should revert.

If you are going to boy scout this one, you could go one step further and turn these into enums.

OutboundWorkflowState.valueOf(stepNumber) == OutboundWorkflowState.REVISE_ITEMS

and create an enum

enum OutboundWorkflowState { 

    CREATE_HEADER(1),
    ADD_ITEMS(2),
    REVISE_ITEMS(3),
    PICK_ITEMS(4),
    PACK_ITEMS(5),
    SEND_SHIPMENT(6)

    Integer stepNumber

    OutboundWorkflowState(int stepNumber) {
        this.stepNumber = stepNumber
    }

}

I am ok with this change, but please make sure you did not miss anything that is still passing the string

a side note - in my opinion it is dangerous to move the import method there because of this @Transactional here - if any issue is detected at the transaction commit, it will be swallowed and we won't eventually display it to a user.

good point, I looked at the methods declared in the picklistController and it looks to be safe to remove the @Transaction from the controller. Most of the transactional logic is handled by the transactional service

Overall I think this looks good to me, but I am slightly afraid, that StockMovementItemsRequestCommand will be confused with the Requisition domain

some potential names to replace the StockMovementItemsRequestCommand

• StockMovementItemsControllerCommand
• StockMovementItemsQueryCommand
• StockMovementItemsParamsCommand
  WDYT?

I think Query or Params is the most suiting here 🤔

TIL: Holy crap, look at this shit ^^^^^^ (had no idea we could do this)

I think there are a few conventions that we could consider here.

When all else fails and we don't have a good idea of a name based on the intent of the command class, then we naming it the same as the method is an ok fallback. I really don't mind this at all, to be honest.

• GetStockMovementItemsCommand

The problem is when the same command is used across methods (like we do here with the details action) we could start thinking of a better name, one that captures the essence of what the command class is intended to be used for.

In this case it probably makes sense to call it something more expressive.

• StockMovementItemQueryCommand (recommended)
• StockMovementItemSearchCommand (meh)
• StockMovementItemFiltersCommand (we've used this in the past, but it might be too narrow)

Note: I think "Request" and "Params" are telling us what the object is, not what it is for. That might come in handy for a base class or when the intention is multi-faceted (we want to query or create with the same command).

We could also just start with something basic like the name of the name of the underlying class and then refactor as needed.

• StockMovementItemCommand

This is generic enough to carry individual fields about a stock movement item for creation as well as properties for filtering/querying.

i've added a convention document for this, so we can update that if/when we decide on something
https://pihemr.atlassian.net/wiki/spaces/OB/pages/3075997703/Command+Class

I am ok with this change, but please make sure you did not miss anything that is still passing the string

@jmiranda what about the label?

I think Query or Params is the most suiting here 🤔