I can't remember if this is true or not, so bear with me.

Is it possible now (or in the future) for us to have validation for different stages of the fullfillment process?

Notes to self:

• If so we might want to might want to break this out into separate validation steps.
• Figure out if this is validating the imported data or a JSON object that was created for a step in the workflow to be validated (i.e. validate the fulfillment request before it is created to the database.

Now we proceed the whole action as one ("full outbound import"), so for now (and I believe forever for this feature) we would have only one validation (not including the one, that validates the file).

I'm not seeing what I wanted to see from this API. It's supposed to be an API to handle fulfillment activities, not the data import. The idea was that we'd have an API to handle the data import process, we'd transform the imported data into requests for the fulfillment API to handle creating and fulfilling the order. But the Fulfillment API was just going to be focused on fulfillment.

Since that's not really the direction we're heading let's just call this ImportPackingListController or something along those lines. I would argue that it's not an API because the input is a multi-part request (file upload).

it doesn't handle the data import. The data import is handled by the PackingListController. Here, we only validate and save the proper JSONs.

This should probably be internal to the ImportPackingListCommand.

I assume the validate is being called when the ImportPackingListCommand is bind'd, but is there any harm in making that explicit here for readability?

I assume if we left the command object as an argument then there's potentially a lot of database queries happening in the line item constraints.

So we'd have to go with the following approach instead.

def validate() { 
    ImportPackingListCommand command = new ImportPackingListCommand(...whatever...)
    if (command.validate() && command.hasErrors()) { 
        response.status = 400
        render([errorCode: 400, errorMessages: command.errorMessages] as JSON)
    }
}

And then in ImportPackingListCommand you can override the getErrorMessages() method run your buildErrors code.

commands are validated automatically, hence adding an additional validate would make the command validate twice, which I believe doesn't have sense and would decrease the performance.
I think it is commonly known that the commands are bound and validated at the bind time. I created a document about that some time in the past (the one about command objects).

Is there a reason we need a separate toTableJson() method? My assumption is that you're adding it because of the rowId, which might feel separate from the other properties in the model (i.e. those coming from the Excel file). If that's true then I would argue that rowId is a fundamental property of ImportPackingListItem. It might be a derived property (an autoincremented index derived by the order in which the imported data row was ingested / processed). But it's still part of the input model. And as such that model has a JSON representation that should be rendered by the toJson() method, the same as a domain.

So just add toJson() methods to ImportPackingList and ImportPackingListItem, then register the toJson as marshallers in Bootstrap.groovy.

And return the whole command object as data.

render([errors: command.errors, data: command] as JSON)

If there's an actual difference between ImportPackingListItem.toJson() and toTableJson() then we probably need a different representation (i.e. a different command and toJson() for what is being rendered via the toTableJson() method).

Tech Huddle Discussion for later

I understand the need for having two

render([errors: errors, data: tableData] as JSON)

We could come up with a convention in which we return a single envelop when we need the data to be returned, in addition to the errors. For example, we could just return the data and the command object would be responsible for rendering its errors.

response.status = 400
render([data: command] as JSON)

The response code tells the client that it should expect either a response with errors or a response with data that may contain errors.

The toJson() would then just need to render the errors.

Map toJson() {
    return [
        errors: hasErrors() ? buildErrors() : []
    ]
} 

No need to implement this now unless it simplies something for you.

Additionally we don't explicitly follow the JSON API specification (https://jsonapi.org). At least not yet. But I would like to move in that direction. And the JSON API says ...

The members data and errors MUST NOT coexist in the same document.

Source: https://jsonapi.org/format/#document-top-level

We still need to solve the problem at hand (i.e. we need both the errors and input data in the response). So I think we could ignore this until/unless we find a better solution.

The table json is needed due to some requirements provided by Kasia and the table itself.
As you know, we bind domains via id, so for example when sending packingList, we provide product ids, so the payload looks like:

{
  packingList: [
    { product: 10001, ...},
    { product; 10002, ...},
  ]
}

so if we were about to rely on this payload to display the data in the table, we would not be able to display the product name, this is where the toTableJson is needed - since we bound the product at the bind time, we return it to the client, so that we can display the: product name, bin location name, expiration date associated with the inventory item in the table:

[
        rowId: rowId,
        product: [
            id: product?.id,
            name: product?.name,
            productCode: product?.productCode
        ],
        binLocation: binLocation?.name,
        palletName: palletName,
        boxName: boxName,
        lotNumber: lotNumber,
        quantityPicked: quantityPicked,
        recipient: recipient?.name,
        expirationDate: expirationDateToDisplay,
        origin: [
            id: origin?.id,
        ],
]

Since this is the PackingListController, we should remove "PackingList" from the method name. However, since "import" is a reserved word, we should call this method upload().

And since we're going to get rid of the FulfillmentApiController for now, let's just move those other methods here. That way everything is together.

Convention: If the method is in a domain, the method name should not include the name of the domain.

Person findByNameOrEmail(String searchTerms)

Editorial Note: We should have lots of tests for this to show what is possible and what is not possible.

Lastly, "regex" seems unintutitive for what is being passed here. The argument is searchTerms not a regular expression, right?

We should be able to remove this as long as we change the render to reference the

render([data: importer.data] as JSON)

https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#deleteWhitespace(java.lang.String)

As I mentioned in the UrlMappings, since the Fulfillments API hasn't been fleshed out as well as I'd like let's make this a little more straightforward by using the PackingListController for everything.

One other thing we can do to improve the code would be to get rid of the file handling above and just use an input stream. This avoids the "bit of mess" and also starts us down the path of removing the file system as a dependency

Tech Debt: The application shouldn't write to the local file system.

The way to do that would be to create a new constructor (or replace the existing one)

PackingListExcelImporter(InputStream inputStream) {
    super()
    read(inputStream)
    excelImportService = Holders.grailsApplication.mainContext.getBean("excelImportService")
}

and then replace the code above with these two lines, maybe with some validation of the command for obvious validation errors like an empty file.

DataImporter dataImporter = new PackingListExcelImporter(command.importFile.inputStream)
render([data: dataImporter.data] as JSON)

As far as I recall, we didn't have that available to us in Grails 1.3.9 but the read(InputStream) is there in AbstractExcelImporter in the Grails 3 version of the plugin.