That URL naming doesn't seem right to me. @jmiranda said a lot about the idea of fetching the expiration date and filling the field after entering the lot, so maybe he has any thoughts about it.

Yeah a few thoughts:

If you were fetching for a single product, it makes sense to make it product centered: /api/products/$productId/inventoryItems or /api/products/$productId/lots.

I originally was thinking you'd want to switch it to be inventory item centric: /api/inventoryItems?productIds=1,2,3. And then you could mask what the frontend is trying to do by turning this into just a basic list endpoint on inventory items:

[{
  'productId': ...,
  'lotNumber': ...,
  'expirationDate': ...,
  'quantityOnHand': ...,
  ...
}]

But that's annoying for you because you want this keyed on product, which is something we reasonably want to support as an API.

So since you're keying on product in the response, it really does make sense to be product centered still: /api/products/inventoryItems?productIds=1,2,3 or /api/products/lots?productIds=1,2,3. It's kinda clunky since you have products twice, once in the uri and once in the query params, but if you think of it as /api/products/inventoryItems fetching all inventory items for all products, then adding productIds=1,2,3 is the filter on that "resource".

And then there's another conversation to be had about whether we want to bother filtering down the fields. Why not just return everything:

{
  'productId123': {
    'productId': 'productId123',
    'lotNumber': ...,
    'expirationDate': ...,
    'quantityOnHand': ...,
  },
  'productId456': { ... },
  ...
}

Not filtering out the other fields by default could make this endpoint more re-usable. If we are worried about performance or payload size (I don't think we are in this case) then we could add a fields=lotNumber,expirationDate query param to filter the selected fields by. But I think that's overkill for now.

Idk, I'm also curious what Justin has to say.

@alannadolny the idea is to have a select for lots, so that you don't have to fill it manually, hence fetching the expiration date after filling in the lot does not satisfy the requirements.

Speaking about the URL, I don't have a strong opinion, but definetely it should contain the inventoryItem, so for now make it at least:

"/api/products/inventoryItems/getLotNumbersWithExpirationDate"

I am thinking about getting rid of get from getLotNumbersWithExpirationDate, because the GET method already says that we are getting the data.

Move that to the createCriteria. You can use projections

It's not only lotNumbers, those are lotNumbers combined with expirationDates

${PRODUCT_API}/getLotNumbersWithExpirationDate
☝️ this should be in the URLs file

Looks weird, move that to a separate function definition or use higher order functions

Can't it be done by passing the appropriate prop to the SelectField component? like: selectLot: true instead of creating a new component?

same question as @alannadolny - how does it differ from the regular selects that it requires a separate component to be created instead of passing proper props?

I’ve been thinking for a while about whether this new component makes sense. In a comment under the ticket, Evan suggested trying a new component that we could start using right away. I think I might have overcomplicated things here and it’s unnecessary, since we can simply pass the required props to the existing component

Why are you casting the type from set to array?

I would suggest changing the name to include "unique" word in some way. I'd also suggest adding a comment explaining that you are collecting unique product ids.

@alannadolny Simply for convenience, because I wanted the unique product IDs as an array rather than a Set, which is an object

not a change request: I am not sure how the type casting works in JS under the hood, I would rather use lodash unique, but I am fine with that version

@alannadolny the idea is to have a select for lots, so that you don't have to fill it manually, hence fetching the expiration date after filling in the lot does not satisfy the requirements.

Speaking about the URL, I don't have a strong opinion, but definetely it should contain the inventoryItem, so for now make it at least:

"/api/products/inventoryItems/getLotNumbersWithExpirationDate"

include product in the variable name - you return product with lot numbers

use return expliciltly + I have a question if it should not return results grouped by product id?
You might have X inventory items per product, hence wouldn't we want to group the lot numbers by the product id, to make it look like:

"10001": [<lot_date_1>, <lot_date_2>],
"10002": [<lot_date_1>, <lot_date_2>],

I think this is what I was suggesting on Slack some time ago

same question as @alannadolny - how does it differ from the regular selects that it requires a separate component to be created instead of passing proper props?

what does this prop do? I haven't seen it previously

The prop creatable is a boolean that determines which SelectType we want to use. When creatable is passed as true, we use Creatable, which allows us to create new options that are not already in the list.

Example usage:

const SelectType = (() => {
  if (creatable) {
    return Creatable;
  }
  if (async) {
    return Async;
  }
  return ReactSelect;
})();

question, no requested change - why JSON.stringify?

I did it this way because without it productIds is an array, and arrays are compared by reference, so I stringify productIds to trigger the effect only when its contents change, not every time a new array instance is created.

when don't we want to refetch the lot numbers then?

For now I implemented it so that we refetch the lot numbers only when entering or returning to the first step, or when clicking Save Progress. In other cases the refetch isn’t needed, because on the second step we can’t change or add a lot number anyway

is is redundant here.
Boolean fields should be named without is in that case, so: lotAlreadyExist is enough. It's method names that include is,
e.g. if you named a field isAvailable, you would have to name the getter getIsAvailable() which is not right. It should be available for the field, and isAvailable() for the method.

moreover if this variable is supposed to return boolean, make it boolean already instead of parsing it via !! later

moreover if this variable is supposed to return boolean, make it boolean already instead of parsing it via !! later

I might have made a mistake here because I actually intended this to be an object that includes both the boolean value and other properties at the same time

add comment what is going on here

doesn't eslint shout that it's in one line?

eslint was screaming earlier, so I had to set it this way

Here it was hard for me to decide whether these two cases should be handled inside useArrowNavigation. I figured it’s probably better to extract them here to avoid making that hook more complicated, and in other places where we’ll use arrow navigation we likely won’t have situations like this, where a field in the middle of a row might be disabled.

Is it used in that component?

We need to pass creatable as true in the useCountStepTable and useResolveStepTable hooks.
Example:

if (fieldName === cycleCountColumn.LOT_NUMBER) {
  return {
    placeholder: isFieldDisabled && translate('react.cycleCount.emptyLotNumber.label', 'NO LOT'),
    options: lotNumbersWithExpiration.map((item) => ({
      id: item.lotNumber,
      name: item.lotNumber,
      label: item.lotNumber,
      value: item.lotNumber,
    })),
    creatable: true,
  };
}

We need to pass this to the SelectField component because our lot number uses SelectField, which internally uses Select.jsx.

In Select.jsx, we use the creatable boolean here:

const SelectType = (() => {
  if (creatable) {
    return Creatable;
  }
  if (async) {
    return Async;
  }
  return ReactSelect;
})();

This allows us to create new options in the dropdown.

So add a comment explaining what this prop is, + make it visible in component props instead of catching it by the spread operator