I added fetching locations and products in one database call to avoid performance issues. At this moment, I tried importing 200 rows. Getting the response took < 500ms, rendering lines on the frontend side took 1s

nice good idea 👍 👍 👍

What happens when we've been given invalid input (ie a product code that doesn't map to a product or a name that doesn't map to a bin)? We still need to be able to return that code/name back to the client so that it can error properly. So I think it's safer to do:

productCode: productMap[row.productCode]?.productCode ?: row.productCode

and same for location:

name: locationMap[row.binLocation]?.name ?: row.binLocation

That way, if we can't resolve it to an entity, id is null (so the client will know it was invalid) but we preserve the name/code that was input by the user so the client can still use it for display.

How should the errors be displayed here? I don't see any point in returning invalid data when we can just display that one error to the user. I am not sure how it is planned to be in the tech design, so maybe I am wrong.

yeah we can talk about it with Manon. I think the desire was for the user to be able to see their invalid input in the table itself, and so be able to correct it in place without needing to reimport.

With products, it might be fine to simply exclude the row and throw an error message up for the user saying they gave an invalid product code "X". Otherwise we'd need some complex logic for displaying invalid products which we don't currently have.

With bins we could be smarter about it though. We could display the invalid bin name that the user input in the table itself and have the column throw a validation error (display value would be "X" but actual value would be null or "INVALID" or whatever) so that the user can see more clearly exactly what they've done wrong and then have re-select a bin from the dropdown in order to proceed.

Doing it the way you have it though, the bin will silently become null, which could still be valid CC input and so there'd be no validation errors. If the user is importing a ton of data, they might not notice that we've changed the bin to null, and they might proceed with the submitting the count, ending up with stock in a bin they didn't intend.

With bins we could be smarter about it though. We could display the invalid bin name...

I think it's a matter of the validation ticket, not this one. But yeah, currently, to display the value of bin location on the select field, we need to have an ID to match the value with the available option from the dropdown. So it's not that easy in case of an invalid bin location. I would rather display an error message above the table saying that you have an error in line X in the file that you imported.

This is what I was afraid of when it was agreed that the import should only take care of parsing the data to JSON. It's getting hard to handle all those edge cases on the frontend, especially I've no idea how we would wanna display (in the table) an error that the product doesn't exist if at this point we wouldn't know which cycle count (which table) we are referring to (let's say a user didn't also provide the cycle count id).

I was thinking about the case that Kacper mentioned. My idea was to display errors related to products just above the table. When a product is not valid we can display it as a "general" error on top of the page as a something not related to any table.

yeah I was imagining it'd be awkward, but in the case of invalid bin, I was hoping it'd be possible to alter the display value for bin to be the user input but then have the actual value of the field be something that represents invalid input. But I haven't actually tried to implement that so it could be impossible. I personally think it's fine for the backed to simply return null if a bin is invalid, as long as we have a good way of notifying to users that their input is lost/modified. The main thing I want to avoid is: the user does an import on bin "ABC" which doesn't exist (they meant to type "ABD", we set the bin to null, they don't notice, then submit the count and suddenly we have stock in the default bin instead of the bin they wanted ("ABD").

Lets talk with Manon about alternative solutions tomorrow.

In the tech design I was assuming that we'd resolve rows using [product code + bin location name + lot number].

From the tech design:

- Loop each row in the json payload and build a map of <String : List> that is keyed on [product + lot + bin] and whose values are the list “matchingXlsRows” containing all the xls (now json) rows with that [product + lot + bin] combination.
- For each entry in the map:
    - Build a list “matchingCountTableRows” by looking up the count table row(s) with a matching [product + lot + bin]. Put the non-custom row at the start of the list (if it exists)
    - If matchingCountTableRows.size == 0, it’s a new item so add all the xls rows in matchingXlsRows[i] to the count table and set newRow=true for each
    - If matchingCountTableRows.size > 0 it’s an existing item so we need to consolidate:
        - While int i < matchingCountTableRows.size && i < matchingXlsRows.size
            - if matchingCountTableRows[i] does not exist, we have more xls rows than existing rows, so simply add the row at matchingXlsRows[i] to the count table and set newRow=true for each
            - if matchingXlsRows[i] does not exist, we have more existing rows than xls rows, so remove the row at matchingXlsRows[i] from the count table.
            - else update the row in the count table at matchingCountTableRows[i] with the data from matchingXlsRows[i]

Doing it this way is slightly more complex to parse but it'd save us from having to pass around cycle count item id in the xls, which is confusing to the user.

But I get that it's simpler to do it the way you have it so I'm open to discuss it. Maybe we can chat at tech huddle on monday.

As I wrote in one of the comments and also on the tech huddle channel - since we allow to import invalid lines (and to add invalid custom lines, that are validated when clicking "Next", we can't make it (productCode + binLocation + lotNumber) a "unique key", because if we did so, we would not be able to add a new custom line with the same productCode, binLocation, lotNumber, and we would eventually update an existing line.
What if a user gave three lines with the same productCode, binLocation, lotNumber? I don't feel comfortable with dealing with such case to be honest, unless we convince Manon that both "save progress" and "next" should behave in the same way in terms of the validation.

That's why in my pseudocode above it maps that key to a list of rows. We wouldn't want to assume it's unique. It then loops through the list of rows matching that key, merging it with existing rows that also have that key (that's what the lines after While int i < matchingCountTableRows.size && i < matchingXlsRows.size is doing).

Lets discuss it in tech huddle tomorrow if you think it won't work

I think we shouldn't actually allow to change the product - it could be further validated, but wanted to say it explicitly.

That sounds a little bit weird. Are you sure?

If you start a cycle count with a product A, you shouldn't be able to suddenly proceed with a cycle count with products A and B (so even though the header in the table would display product A, you might suddenly have a row with a different product).

So this case can happen if the row in the xls has the same cycle count item id, but the product id/code was changed in the xls for some reason?

I still think we can implement this without needing cycle count item id, so this wouldn't be a problem here but we can discuss.

But in case we keep count item id, I think if the count item id is the same but the product id is not the same, then we should probably error. I'm worried that it's not safe to assume that the user meant to put a different product in that row. They don't know what cycle count item id means (which is another reason why I don't like having it in the xls) so I don't think we can use cycle count item id alone as the "source of truth" for identifying matching rows. Product id must also match otherwise something strange has happened in the xls.

But again, I'd much rather not have cycle count item id in the xls at all

As I wrote in one of the comments and also on the tech huddle channel - since we allow to import invalid lines (and to add invalid custom lines, that are validated when clicking "Next", we can't make it (productCode + binLocation + lotNumber) a "unique key", because if we did so, we would not be able to add a new custom line with the same productCode, binLocation, lotNumber, and we would eventually update an existing line.
What if a user gave three lines with the same productCode, binLocation, lotNumber? I don't feel comfortable with dealing with such case to be honest, unless we convince Manon that both "save progress" and "next" should behave in the same way in terms of the validation.

could you explain with an example what's the purpose of that? If a corresponding item exists, we should update its values instead of removing it, but maybe I don't understand the point of those lines?

The purpose of that is to have only items that are new or have a wrongly entered ID. So when a user enters something wrong, it will be treated as a new row.

This is what I was afraid of when it was agreed that the import should only take care of parsing the data to JSON. It's getting hard to handle all those edge cases on the frontend, especially I've no idea how we would wanna display (in the table) an error that the product doesn't exist if at this point we wouldn't know which cycle count (which table) we are referring to (let's say a user didn't also provide the cycle count id).