Tags - Basic
(UXPROD-250)
|
|
| Status: | Closed |
| Project: | mod-inventory |
| Components: | None |
| Affects versions: | None |
| Fix versions: | 18.2.1, 19.0.0 | Parent: | Tags - Basic |
| Type: | Bug | Priority: | P3 |
| Reporter: | Ian Walls | Assignee: | Volodymyr Rohach |
| Resolution: | Done | Votes: | 0 |
| Labels: | epam-folijet, folijet-support, has-releasenote-lotus, has-testrail, regression, support | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original estimate: | Not Specified | ||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||
| Issue links: |
|
||||||||||||||||||||||||||||||||||||||||||||
| Sprint: | Prokopovych - Sprint 141, Prokopovych - Sprint 137, Prokopovych - Sprint 138, Prokopovych - Sprint 139, Prokopovych - Sprint 140, Prokopovych - Sprint 142 | ||||||||||||||||||||||||||||||||||||||||||||
| Story Points: | 3 | ||||||||||||||||||||||||||||||||||||||||||||
| Development Team: | Folijet Support | ||||||||||||||||||||||||||||||||||||||||||||
| Release: | Morning Glory (R2 2022) Bug Fix | ||||||||||||||||||||||||||||||||||||||||||||
| Affected Institution: |
!!!ALL!!!
|
||||||||||||||||||||||||||||||||||||||||||||
| Tester Assignee: | Ann-Marie Breaux (Inactive) | ||||||||||||||||||||||||||||||||||||||||||||
| Epic Link: | Tags - Basic | ||||||||||||||||||||||||||||||||||||||||||||
| RCA Group: | Legitimate regression | ||||||||||||||||||||||||||||||||||||||||||||
| Description |
|
Overview: When attempting to add a tag to a MARC-backed Instance that has a preceding or succeeding title entry (780 or 785) that does not link to another existing instance, the UI returns the error: Cannot save tag data to record. Check that you have the required permissions. Behind the scenes, this is returned by the API endpoint: {{{}} If the preceding/succeeding title entry is linked to another Instance, this error does not occur. Steps to Reproduce:
Alternate steps to reproduce:
Expected Results: The tag should add. Actual Results: Error message; see above for dev error message, and attached for UI error message Interested parties: St Vincent College NOTE Reproduces on Juniper Bugfest, Kiwi Bugfest, and current hosted ref envs. Checked tags on holdings and items associated with the problematic instance, but no problems with them. Only bug is for instances. |
| Comments |
| Comment by Ann-Marie Breaux (Inactive) [ 08/Dec/21 ] |
|
Hi Ian Walls My inclination is not to push this into Kiwi as a Bugfix or Hotfix, but to fix in Lotus. Do you think that will be acceptable to St. Olaf? |
| Comment by Ann-Marie Breaux (Inactive) [ 08/Dec/21 ] |
|
Holly Mistlebauer and Charlotte Whitt UITAGS is owned by Prokopovych. I think this bug is UI-only. Will Prokopovych have time to work on this during Lotus? If not, I'll ask another team. Thank you! |
| Comment by Ian Walls [ 08/Dec/21 ] |
|
Having to wait until the summer to be able to tag Instances is not going to sit well with St. Vincent, I'm sure. I think I can do a temporary, un-durable fix by removing the bad entries from the preceding_succeeding_title table... I think this would let them tag the Instances at least until they did an edit in quickMARC. I'll follow up and see if this measure will provide some relief. |
| Comment by Ann-Marie Breaux (Inactive) [ 09/Dec/21 ] |
|
Hi Ian Walls I understand, but I imagine this would be a small minority of their instances. One other possible solution would be to (temporarily) create a statistical code type and stat codes with the tags they would have assigned. I realize that's a hack, but maybe it would help? Charlotte Whitt and Holly Mistlebauer what's your assessment of Prok being able to cover this bug fix? |
| Comment by Ann-Marie Breaux (Inactive) [ 15/Feb/22 ] |
|
Hi Khalilah Gambrell Prok removed this from Lotus. Now that Spitfire owns Tags, could one of the Spitfire devs perhaps take a look at it? |
| Comment by Khalilah Gambrell [ 15/Feb/22 ] |
|
Sure, we can review for Morning Glory Ann-Marie Breaux but I will add the label [delegate_candidate] in case another team can address sooner. |
| Comment by Ann-Marie Breaux (Inactive) [ 15/Feb/22 ] |
|
Perfect - thanks, Khalilah Gambrell - and if my folks can figure out the weird import profile tags problem, I'll ask them to look at this one too. |
| Comment by Charlotte Whitt [ 21/Feb/22 ] |
|
Support SIG: Should this ticket be assigned the release of Morning Glory ? |
| Comment by Ann-Marie Breaux (Inactive) [ 21/Feb/22 ] |
|
Hi Charlotte Whitt - yes, I've added it. |
| Comment by Denys Bohdan [ 31/Mar/22 ] |
|
Pavlo Smahin are any of these Instance fields updated when trying to add a tag to instance? |
| Comment by Pavlo Smahin [ 31/Mar/22 ] |
|
Ann-Marie Breaux, Khalilah Gambrell, this is not related to mod-tags. This error comes from mod-inventory and is related to the Instance update. So I suggest moving it to the Folijet BE team. |
| Comment by Charlotte Whitt [ 04/Apr/22 ] |
|
Marc Johnson - I notice this (by someone) has been assigned to the Prokopvych team (Morning Glory work). Does this makes sense to you too? |
| Comment by Marc Johnson [ 04/Apr/22 ] |
Ann-Marie Breaux assigned the work originally, Khalilah Gambrell re-assigned it back from Spitfire. They will likely have more context than I do as to the reasoning. I agree with Pavlo Smahin assessment that this is likely an issue with how mod-inventory checks for blocked fields with MARC backed instances. As to whether it is Prokopovych or Folijet that is responsible for that area, I don't know, I think that's probably a bit of a grey area. The Prokopovych team likely can investigate the issue. |
| Comment by Ann-Marie Breaux (Inactive) [ 07/Apr/22 ] |
|
Hi Charlotte Whitt and Marc Johnson Do you think this broke when the linked preceding/succeeding titles were coded? It's not clear to me if it used to work and broke (regression) or if it never worked after the linked preceding/succeeding, but no one noticed it until now. |
| Comment by Marc Johnson [ 08/Apr/22 ] |
I don't think I can answer that conclusively. At least not without significant investigation. I did a little investigation and found that there were changes made to how blocked fields are evaluated specifically for parent / child instances as part of
In that issue, you
This statement (in the description of this issue) leads me to speculate that this is a different / related example to that bug. I wonder if
|
| Comment by Marc Johnson [ 08/Apr/22 ] |
|
Ann-Marie Breaux Ian Walls Charlotte Whitt I suspect this is not related to changing tags, and can likely be triggered by any change to an unprotected field. Has the same scenario been tested when making changes to another unprotected field? |
| Comment by Ann-Marie Breaux (Inactive) [ 08/Apr/22 ] |
|
Oh - that's an interesting idea, Marc Johnson For source = MARC Instances, these are the fields NOT controlled by the underlying SRS MARC Bib:
Ian Walls do you want to try editing a couple of the above fields, to see if the tags issue reproduces for them? |
| Comment by Ian Walls [ 08/Apr/22 ] |
|
Editing Suppress from Discovery, Staff Suppress, Previously Held, Cataloged Date, Instance Status Term, Statistical Code, Administrative Note or Nature of Content does NOT cause a similar error message. This appears to be limited only to Tags. |
| Comment by Ann-Marie Breaux (Inactive) [ 11/Apr/22 ] |
|
Thank you for checking, Ian Walls Marc Johnson or Charlotte Whitt any other questions. or all set as far as fixing in Morning Glory. Would you like me to add this as a Known issue in the Lotus release notes, or will one of you do it? |
| Comment by Marc Johnson [ 11/Apr/22 ] |
Thank you for testing those scenarios. It surprises me that it is specific to tags.
I think this can be investigated. |
| Comment by Marc Johnson [ 11/Apr/22 ] |
|
I tried to recreate this issue manually (before trying to write an automated test).
I first picked an MARC source instance and added a 780 \ \ $a 5438507564 entry in the quick MARC UI. This did not seem to create a preceding / succeeding title. Did I do something wrong?
There doesn't seem to be a file attached to this issue. Please can you help with this? |
| Comment by Ann-Marie Breaux (Inactive) [ 11/Apr/22 ] |
|
Hi Marc Johnson I've attached the file that Ian Walls referenced. Just imported it to folio-snapshot and confirmed that I could not add a new tag to the first instance created from the file. Here's the log info for that imported file, in case you want to look at it: https://folio-snapshot.dev.folio.org/data-import/job-summary/1fc7a356-4443-4753-8516-a5f9f4f10882?sort=recordNumber |
| Comment by Marc Johnson [ 11/Apr/22 ] |
Thank you, that's great. I'll use that when I swing back round to this issue. |
| Comment by Marc Johnson [ 12/Apr/22 ] |
|
Ann-Marie Breaux Ian Walls Jenn Colt I've done some initial investigation into this. This issue is caused by a side effect of the changes made in
Blocked fields for MARC source instances is implemented by comparing the values of these fields between the stored record representation and the representation sent in the request to update the instance. This works reasonably well for fields that are directly part of the instance. However, preceding / succeeding titles are not directly part of the instance, due to them possibly being a (bi-directional) link between instances. The work done in
Viktor Draban Ann-Marie Breaux Jenn Colt How did we arrive at this solution? Removing these only from the stored representation works when editing general fields from the UI, because the UI happens to not send the ID property of the preceding / succeeding titles. Michal Kuklis Zak Burke Matt Connolly Is this because the UI constructs a new representation of the instance during a regular edit? Unfortunately, it does not work when adding a tag, because the ID of the preceding / succeeding title is included by the UI for that operation. Had I have realised that tags were changed on instances in a fundamentally different user experience to a regular edit, this might not have been as much of a surprise to me. I can fix the current issue by removing the same IDs from the stored representation. However I'm concerned that will introduce other issues or degrade the blocking of editing of preceding / succeeding titles further. I'm already a little surprised that this approach doesn't break when a linked preceding / succeeding title is edited in the UI. Although maybe that's because the UI stops this from happening without the involvement of the back end? Thoughts? |
| Comment by Marc Johnson [ 13/Apr/22 ] |
|
I got some feedback yesterday that what I wrote above was not easy to follow and did not offer what I think could be done. I'm going to try a more direct comment in case that helps folks understand better. Apologies for the (second) long comment. There is a lot going on in this issue and I think it's important folks understand it before we decide what to do. Please let me know if this helps folks understand both the cause of the issue and how we got here better. (My inferred) HistoryExposure of the preceding / succeeding record IDThe preceding / succeeding title relationship is considered a separate record to the instance. This is because it may relate two separate instances (if linked / connected) together (and thus is part of both of them and so cannot be part of only one of them). This is exposed as part of the storage APIs (due to FOLIO's storage / business logic split architecture). Whilst the mod-inventory API attempts to hide this by allowing these relationships to be changed by changing either of the instances, unfortunately it reveals this modelling by including the ID of the relationship record in the representation of an instance. Blocking field editing for MARC source instancesA set of fields (or nested sets of fields .e.g. preceding / succeeding titles) is not allowed to be changed when an instance has the source MARC (and is meant to be associated to a MARC source record). This was implemented by fetching the stored representation of an instance (before edit) and comparing it (as JSON) to the proposed edited instance representation (from the client, in this case the UI). This means that any difference between these JSON representations will cause the block validation error. Removal of the ID when editing (via the UI)When an instance is edited in the UI, the ID properties of each preceding / succeeding title relationship is removed. In effect, the UI does edit the preceding / succeeding titles in an instance (meaning that arguably the block validation error was correct). I believe this is probably because edit pages in the UI tend to construct a brand new representation of a record (the instance) from what is presented in the edit form. Meaning that anything not presented is lost in the new representation. How this breaks field edit blocking?As the comparison between stored and proposed edit representation uses JSON and compares all fields and nested fields, the absence of the ID property in the proposed edit triggers the block validation error. The workaround to allow that to happen
This means that the comparison does not find a difference when editing from the UI. (it also removes the two instance ID fields from that representation too, I don't yet know why) Why that breaks tag editing?When adding (and presumably removing) a tag, the UI includes the ID of a preceding This would likely also break other clients which did not implicitly edit the instance as the edit page UI does now. AnalysisDuring
This workaround in the blocking logic for a (arguable unintentionally bad acting) client that edited (what should be) a protected field effectively endorsed this kind of edit. Unfortunately, that also meant that a well behaved client that did not edit the preceding / succeeding title relationship is considered to be editing it. This is because the ID is present in the proposed edit however is not in the amended representation from storage. Unanswered QuestionsI don't yet know why:
Options for ResolutionChange how editing in the UI works (and remove the workaround)
Expand the workaround
I have this change ready to go. It seems to resolve this immediate issue without breaking the original workaround (according to the test written specifically for it). However, I am concerned that this may increase the impact on clients other than the edit page and / or undermine parts of the blocking logic because it will effectively allow these edits of a preceding / succeeding title to occur. I don't think that can be recreated via the UI because it already blocks it. Rework how blocking edits of protected fields workPart of the reason for the workaround is due to how the current implementation compares JSON representations naively. We could change how that is implemented to allow for greater control (and better feedback) by doing the comparison in a different way. This is likely the highest effort approach and potentially has other knock on effects. |
| Comment by Charlotte Whitt [ 20/Apr/22 ] |
|
Hi Marc Johnson - that is an excellent write up I hear your concerns re. the suggested expansion of the work around. Do we need to gather a group of people (incl. Data Import developers) to make the decision of which resolution we are going to aim for, or how would be the best way to move forward? |
| Comment by Ann-Marie Breaux (Inactive) [ 20/Apr/22 ] |
|
Hi Marc Johnson and Charlotte Whitt Whatever you think is best, as far as if we need to get together to discuss. I haven't digested everything you've written above, but will look at it more closely later. In the meantime, one issue (I think) is that the Preceding/Succeeding titles are the ONLY fields in the Instance that are both 1) controlled by the underlying SRS (if there are 780s/785s in the SRS MARC), but also can have titles added manually by editing the instance (for source = MARC where there are no 780s/785s in the SRS MARC). That duality (I think) adds a complexity we do not face with any other Instance field. |
| Comment by Marc Johnson [ 20/Apr/22 ] |
Thank you
I was hoping that folks would engage with my comments above. I don't know how urgently folks want to move this forward, so I haven't been pushing it. I'm happy to be in a meeting if needed. |
| Comment by Marc Johnson [ 20/Apr/22 ] |
Thank you.
That is really interesting. I wasn't aware that folks could legitimately add preceding / succeeding titles to a MARC source instance. How did we come to allow that? How does that work when the current implementation includes preceding / succeeding titles in the set of blocked fields? (maybe this partly explains the workaround that was put in place) |
| Comment by Charlotte Whitt [ 20/Apr/22 ] |
|
Yes, Marc Johnson and Ann-Marie Breaux - re.
Just checked: It is blocked:
|
| Comment by Ann-Marie Breaux (Inactive) [ 20/Apr/22 ] |
|
I'm in a long mtg this morning. I'll take a closer look at it later today - apologies if I have added to some confusion |
| Comment by Ann-Marie Breaux (Inactive) [ 20/Apr/22 ] |
|
Hi Marc Johnson and Charlotte Whitt I doublechecked and clearly misremembered about preceding/succeeding being editable in Instances where source = MARC. There was an old bug about that, but it was fixed. I'm not sure if reviewing some of the resolved bugs in this filter might be helpful: https://folio-org.atlassian.net/issues/?jql=text%20~%20%22preceding%20succeeding%20instance%22%20and%20issuetype%20%3D%20bug In particular:
|
| Comment by Marc Johnson [ 20/Apr/22 ] |
Thank you for clarifying that.
I think a review of that issue is a good idea. That is the introduction of the workaround that I refer to in my earlier comments. It would be helpful, for me, to understand why this workaround was introduced to address this issue, given that one of my options is to remove that workaround (as it is that workaround that causes this issue). |
| Comment by Oleksiy_Lemeshko [ 16/May/22 ] |
|
Ann-Marie Breaux there are still questions opened related to this ticket. shall we have a slack chat or a call to finalize the decision about it?
|
| Comment by jroot [ 23/May/22 ] |
|
Just coming here to chime in we've hit this at Tamu. Took me forever and a day to track it down... On Juniper HF6 |
| Comment by Charlotte Whitt [ 01/Jun/22 ] |
|
Ann-Marie Breaux - will it be helpful if we set up a meeting and together with one/two of FOLJIET developers and Marc Johnson try to figure out how to solve this bug. As I wrote in the description I can also reproduce in Bugfest Lotus - see Instance HRID: in3596955 |
| Comment by Charlotte Whitt [ 20/Jun/22 ] |
|
Support SIG: Ann-Marie Breaux please see the comment above. This ticket is blocked, and it will be helpful if we can schedule a talk. Please let me know when you and your developers will be available, then us from the Prokopovych team will make room For now I changed the release to Morning Glory Bugfix instead of Morning Glory. |
| Comment by Ann-Marie Breaux (Inactive) [ 20/Jun/22 ] |
|
Kateryna Senchenko Could you or one of the BE devs take a look at this? If it would be helpful for Folijet dev and Prokopovych dev to look at it together, let's try to do that this week. Thank you! |
| Comment by Kateryna Senchenko [ 21/Jun/22 ] |
|
Hi Ann-Marie Breaux, Marc Johnson, I agree we should probably discuss how blocking of editing preceding / succeeding titles should be implemented. The expanding of the workaround that Marc Johnson prepared does appear to solve the problem, but if I understand it correctly would shift the validation logic (weather preceding / succeeding titles can or cannot be updated) to UI and that is not ideal. I'll consult with Mariia Aloshyna on how the edit is implemented from the UI and we'll probably have a bit more understanding till the meeting. Ann-Marie Breaux, would you organise a meeting? Thank you very much! |
| Comment by Marc Johnson [ 21/Jun/22 ] |
I'm not sure if I expressed this above. Ideally, I think the workaround should be removed. I think it was added due to a quirk in how the UI was implemented. And bringing the edit UI into line with the tags UI (in terms of what they send to the back end) could simplify both. My caution around advocating for that is I don't know why the workaround was put in place originally or if I'm unaware of a constraint with how we manage hidden properties in forms in FOLIO. |
| Comment by Kateryna Senchenko [ 21/Jun/22 ] |
Thank you Marc Johnson, I agree. I believe the workaround was implemented as a very quick and simplest solution (considering it was hotfixed) and all the cases/scenarios were not properly thought through. So yes - it would be good to work out a solution that would allow to remove the workaround. We'll review the UI side of this process as well |
| Comment by Charlotte Whitt [ 21/Jun/22 ] |
|
Kateryna Senchenko - excellent. Please include me in the meeting too. Thanks |
| Comment by Ann-Marie Breaux (Inactive) [ 22/Jun/22 ] |
|
Discussed with Marc Johnson Kateryna Senchenko Charlotte Whitt Mariia Aloshyna Has 2 tickets:
See https://folio-org.atlassian.net/browse/MODINV-429 for additional background Decide whether Prokopovych or Folijet will do both the MOD and UI Jiras (after input from Masha and Mikhail) |
| Comment by Ann-Marie Breaux (Inactive) [ 22/Jun/22 ] |
|
Michal Kuklis and Mariia Aloshyna These Jiras may also be useful to review for additional background:
|
| Comment by Charlotte Whitt [ 27/Jun/22 ] |
|
Marc Johnson - can I unblock this ticket now? CC: Ann-Marie Breaux - did you and the Foljiet devs decide that you want to do the work, or are we at the Prokopovych team to take it on. Thanks. |
| Comment by Marc Johnson [ 27/Jun/22 ] |
Given the need to closely coordinate the front end and back end changes needed to resolve this, I don't think this should be unblocked until we have:
|
| Comment by Ann-Marie Breaux (Inactive) [ 28/Jun/22 ] |
|
Hi Marc Johnson and Charlotte Whitt I think Mariia Aloshyna has confirmed that Folijet can take the UI issue. Masha - if so, please confirm with Kateryna Senchenko that we'll take this BE issue as well, and we'll need to relink to the proper Folijet features. It would be good if we could get these changes in place before the MODINV and UIIN releases, but if not, we'll do bugfix releases for MG before Bugfest starts. |
| Comment by Marc Johnson [ 28/Jun/22 ] |
Unfortunately, mod-inventory was released yesterday after having been unblocked. |
| Comment by Kateryna Senchenko [ 28/Jun/22 ] |
|
Hi Ann-Marie Breaux, Marc Johnson, Mariia Aloshyna and I have discussed the issue and think we can take care of this one and
Moving it to Folijet team. Thank you! |
| Comment by Ann-Marie Breaux (Inactive) [ 28/Jun/22 ] |
|
No problem, Marc Johnson Folijet will do a MODINV Bugfix release when UIIN is ready |
| Comment by Marc Johnson [ 29/Jun/22 ] |
|
Ann-Marie Breaux Mariia Aloshyna Kateryna Senchenko Thank you for taking this work on. |
| Comment by Ann-Marie Breaux (Inactive) [ 12/Jul/22 ] |
|
Hi Maksym Rutkovskyi Could you create a TestRail for this bug, using the Alternate steps to reproduce in the above description, plus the attached MARC file. Thank you! |
| Comment by Maksym Rutkovskyi [ 22/Jul/22 ] |
|
Hi Ann-Marie Breaux! Work as expected on FOLIO Google Chrome Version 101.0.4951.67 (Official Build) (64-bit) Tested and the bug is not reproduced. The tag is now successfully assigned to the instance and there are no errors in the process when creating or selecting an existing tag. See the attached: Unable to render embedded object: File (Checking state saving for Checkbox and Filter (2).mp4) not found. |
| Comment by Oleksii Petrenko [ 25/Jul/22 ] |
|
Deployed to MG bf. Please proceed with verification. |
| Comment by Ann-Marie Breaux (Inactive) [ 01/Aug/22 ] |
|
Hi Volodymyr Rohach and Maksym Rutkovskyi Looks good on Morning Glory Bugfest for alternate repro steps (which is covered by TestRail C358144). Looks like we may have had a regression for bugfix
|
| Comment by Ann-Marie Breaux (Inactive) [ 02/Aug/22 ] |
|
No bug after all! I was manually editing the quickMARC using 780 or 785 $a instead of $t. Once I corrected the subfield, all looks fine. Creating the additional TestRails and linking to this bug. All is fine! |