[UIIN-2322] No error message when trying to save a value with 'null'; e.g. as seen in Statistical code field on an holdings, instance, and item record Created: 19/Dec/22 Updated: 07/Feb/24 Resolved: 20/Mar/23 |
|
| Status: | Closed |
| Project: | ui-inventory |
| Components: | None |
| Affects versions: | None |
| Fix versions: | 9.3.0 |
| Type: | Bug | Priority: | P3 |
| Reporter: | Thomas Trutt | Assignee: | Michal Kuklis |
| Resolution: | Done | Votes: | 0 |
| Labels: | support | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original estimate: | Not Specified | ||
| Attachments: |
|
||||||||||||||||||||||||
| Issue links: |
|
||||||||||||||||||||||||
| Sprint: | Prokopovych - Sprint 158, Prokopovych - Sprint 159, Prokopovych - Sprint 160, Prokopovych - Sprint 161 | ||||||||||||||||||||||||
| Development Team: | Prokopovych | ||||||||||||||||||||||||
| Release: | Orchid (R1 2023) Bug Fix | ||||||||||||||||||||||||
| Affected Institution: |
!!!ALL!!!
|
||||||||||||||||||||||||
| Tester Assignee: | Charlotte Whitt | ||||||||||||||||||||||||
| RCA Group: | Incomplete/missing requirements | ||||||||||||||||||||||||
| Description |
|
Overview: Several data properties, in Inventory (instance, holdings, item) and maybe in more apps (but this has not been investigated further), here properties with value 'null' can be saved, without an error message is displayed. E.g.: When editing an item record if the use clicks on 'Add statistical code' a null statistical code is added to the item record. If the user does not remove this code nor selects an option form the dropdown the save fails silently in the background. The save response returns with code 422 with the following payload:
{
"errors":[
{
"message":"elements in list must match pattern",
"type":"1",
"code":"elements in list must match pattern",
"parameters":[
{
"key":"statisticalCodeIds",
"value":"[null]"
}
]
}
]
}
Steps to Reproduce:
Expected Results: Actual Results: Additional Information: That is the case here in src/Item/EditItem/EditItem.js and src/Item/CreateItem/CreateItem.js: both use the useItemMutation hook but fail to either (a) pass an onError callback to the hook or (b) provide a catch clause when calling mutateItem. |
| Comments |
| Comment by Charlotte Whitt [ 19/Dec/22 ] |
|
Thomas Trutt - thanks for filing this issue. It's apparently a broader issue, while I see different errors/error messages for also holdings when saving statistical codes with empty content [null] and also instances. Also when I try to save an empty administrative code. Is this an issue in other apps as well, or have you only seen it in Inventory? For now I'll put this ticket in Draft, until we know if this can be solved in a more general way, but I would need to check with the UI developers, to get a sense of what are the options. |
| Comment by Thomas Trutt [ 20/Dec/22 ] |
|
I have not looked in other apps, or other fields. This came up in our move to MG on Friday. We had item records that had null Statistical codes attached to them that where causing issues in with check-in. We cleaned up those records and I brought this Erins attention as a potential bug for the check-in app. When she tried to duplicate it she note that the record wouldn't save but there was no error message. I then confirmed this on the Nolana test env and wrote this up. I can go through and test the other fields and different record levels to see where the errors are not being caught. I will also check the other apps. If it is a larger issue perhaps the stripes is not requesting 422 as an error code? It may take me a few days to get around to it though.. |
| Comment by Erin Nettifee [ 20/Dec/22 ] |
|
Charlotte Whitt Thomas and I discussed this yesterday as it was discovered through impacts on checking in items. I'm not aware of other apps that are using statistical codes in the same way that Inventory uses them. Do you know if other apps are using this? |
| Comment by Charlotte Whitt [ 09/Jan/23 ] |
|
Erin Nettifee Thomas Trutt - my questions was not so much re. Statistical codes, but the possibility to save records having a Null value |
| Comment by Anya [ 23/Jan/23 ] |
|
Support: any progress on this ticket - can we assign a release target? Thanks |
| Comment by Charlotte Whitt [ 25/Jan/23 ] |
|
Anya - no unfortunately not. Erin Nettifee and Thomas Trutt did you have a change to check if this is a much more general issue than just Statistical codes? |
| Comment by Erin Nettifee [ 25/Jan/23 ] |
|
Charlotte Whitt I'm not even sure how to start figuring that out - I don't know what in FOLIO would control it? I know in Users, if you don't put a value into the field, the record just doesn't have the attribute, e.g., if I don't add a phone number for a person, FOLIO doesn't save that as "phone number: []" it just doesn't save a phone number attribute in the record at all. I think in this case, what should be happening is like what happens when you accidentally click an item check in note, but you don't add anything. you can't save the record. |
| Comment by Anya [ 06/Feb/23 ] |
|
Support: Zak Burke / Khalilah Gambrell this seems like it is a wider issue with FOLIO how can this be solved everywhere? |
| Comment by Erin Nettifee [ 06/Feb/23 ] |
|
I disagree that this is necessarily a wider issue in FOLIO. Other areas of the item record - like the item check out note - properly parse the error message if you happen to leave one blank. This is just a part of the data where the error is not properly parsed. It should be prioritized like any other bug. |
| Comment by Zak Burke [ 06/Feb/23 ] |
|
Anya: while the problem may be a "wider issue with FOLIO", there is not a "one stop shop" fix. Essentially, any given request may succeed or fail, and the problem is that some request-handlers do not check the response status to make sure it is successful. IOW, the "problem" is not with particular code but with a particular pattern. That is the case here in src/Item/EditItem/EditItem.js and src/Item/CreateItem/CreateItem.js: both use the useItemMutation hook but fail to either (a) pass an onError callback to the hook or (b) provide a catch clause when calling mutateItem. |
| Comment by Charlotte Whitt [ 06/Feb/23 ] |
|
Michal Kuklis - do you have enough information based on Zak's input, then I can update the story line and the description to reflect, that this is to be solved every where in instance, holdings, item where where we have a similar issue |
| Comment by Michal Kuklis [ 06/Feb/23 ] |
|
Charlotte Whitt I honestly do not know because this code was written by a different team in the past (https://folio-org.atlassian.net/browse/UIIN-1867). Since this is not using stripes-connect (and instead react-query) I think we should be able to setup a global onError handler as Zak suggested. If that's how we want to handle it then this issue belongs to stripes-core and not ui-inventory. This will only handle it for queries which are using react-query currently. I assume for other queries using stripes-connect this is already handled correctly (by showing a big alert). |
| Comment by Charlotte Whitt [ 07/Feb/23 ] |
|
Thanks a lot Michal Kuklis - I'll move this ticket to stripes-core, and update the description. |
| Comment by Michal Kuklis [ 07/Feb/23 ] |
|
Charlotte Whitt thank you. I do believe that we also should handle this situation a bit differently from the ui-inventory point of view and perhaps filter out the empty values before the save happens. What do you think? |
| Comment by Charlotte Whitt [ 08/Feb/23 ] |
|
Hi Michal Kuklis - that sounds reasonably. Then you probably need a ticket for this. |
| Comment by Michal Kuklis [ 08/Feb/23 ] |
|
Charlotte Whitt I moved this back to ui-inventory. I spent a bit more time looking at the code there and it looks like this issue is already partially handled there (for example it works fine for instances). I'm going to poke around this and will fix it for both holdings and items in the inventory. Zak Burke and I are also experimenting with another approach in stripes-core under: https://folio-org.atlassian.net/browse/STCOR-687 |
| Comment by Charlotte Whitt [ 09/Feb/23 ] |
|
Totally awesome Michal Kuklis - thank you so much. |
| Comment by Charlotte Whitt [ 20/Mar/23 ] |
|
Manual test in FOLIO Snapshot, version @folio/inventory 9.5.1000003643, using Chrome. When saving an instance where the Statistical code is empty (NULL) - then the record can not be saved. I'm not quite sure the error message though is what a user would expect - what do you say Thomas Trutt? When saving a holdings with empty data for the statistical code, this is still happening, so this part of the work, is not yet solved. Finally when saving an item with empty data for the statistical code, then the error message is not looking in any understandable form for a staff user. But it is preventing save of the record, and the record can first be saved, when the empty 'row' has been deleted by clicking the trash icon. Michal Kuklis - I'll put this ticket back In progress. |
| Comment by Michal Kuklis [ 20/Mar/23 ] |
|
Charlotte Whitt unfortunately there is nothing else I can do about this. This is a generic error returned by the server and the frontend is just handling it by showing it in the modal. |
| Comment by Charlotte Whitt [ 20/Mar/23 ] |
|
Michal Kuklis - Instance, Holdings and Item save of empty strings - are handled in three different ways. That seems a bit odd, I think. |
| Comment by Michal Kuklis [ 20/Mar/23 ] |
|
Charlotte Whitt I agree but that will have to change on the server. |
| Comment by Thomas Trutt [ 21/Mar/23 ] |
|
I would agree Charlotte Whitt that the error messages do not make sense. I also understand what Michal Kuklis is saying that the front end is just passing the error along form the backend. What would you suggest? |
| Comment by Michal Kuklis [ 21/Mar/23 ] |
|
Thomas Trutt ideally the responses from the server from all these 3 endpoints (items, holding records and instances) should have the same format. |
| Comment by Charlotte Whitt [ 21/Mar/23 ] |
|
I can file that as a new story, instead of a bug, if we can agree on what the text should be. My suggestion would be: Message from the back end:
Message in the UI: The UX example above is following the pattern for when an Resource identifier type is missing: Any thoughts Thomas Trutt Michal Kuklis |
| Comment by Thomas Trutt [ 21/Mar/23 ] |
|
I think that is fine. You could even truncate it and do: "Statistical code can not be blank/empty" |
| Comment by Charlotte Whitt [ 21/Mar/23 ] |
|
Excellent Thomas Trutt - I'll write that up. |