[MODUIMP-62] Mod-user-import throws an error if users are assigned a department that is not part of the first ten departments in a /departments API call Created: 03/Feb/22 Updated: 25/Oct/22 Resolved: 08/Feb/22 |
|
| Status: | Closed |
| Project: | mod-user-import |
| Components: | None |
| Affects versions: | 3.6.3 |
| Fix versions: | 3.6.4 |
| Type: | Bug | Priority: | P2 |
| Reporter: | Theodor Tolstoy (One-Group.se) | Assignee: | Adam Dickmeiss |
| Resolution: | Done | Votes: | 0 |
| Labels: | support | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original estimate: | Not Specified | ||
| Attachments: |
|
||||||||
| Issue links: |
|
||||||||
| Sprint: | CP: sprint 133 | ||||||||
| Story Points: | 2 | ||||||||
| Development Team: | Core: Platform | ||||||||
| Affected Institution: |
!!!ALL!!!
|
||||||||
| RCA Group: | TBD | ||||||||
| Description |
| Comments |
| Comment by Theodor Tolstoy (One-Group.se) [ 03/Feb/22 ] |
| Comment by Theodor Tolstoy (One-Group.se) [ 03/Feb/22 ] |
|
Here is a comparison between how the data is fetched from FOLIO in the Java code: mod-user-import can handle 2147483637 more groups than departments... |
| Comment by Holly Mistlebauer [ 03/Feb/22 ] |
|
Theodor Tolstoy (One-Group.se): Does this need to be a hot fix to Kiwi? Thanks... |
| Comment by Zak Burke [ 03/Feb/22 ] |
|
Is it possible to pre-parse the input file to find the unique patron group values so we can (a) count them and (b) feed that uniq'ed list to a query that will retrieve them all? We've done things like this in ui-users to correctly set the scope of queries for loan policies given a list of a user's loans. Or do we just buckle and take the cheap fix, throwing limit=1000 on the patron-groups query and hope that's big enough to get all the patron groups and small enough that nothing blows a gasket? |
| Comment by Molly Driscoll [ 04/Feb/22 ] |
|
This bug also impacts the departments filter in the Users app. Do we need a separate Jira for this or will that be fixed as soon as this bug is fixed? I would push for this to be included in a Kiwi hotfix because we have several libraries we are implementing planning to use departments both as part of initial migration and for ongoing user loading. |
| Comment by Kyle Felker [ 04/Feb/22 ] |
|
The mod-user-import endpoint is defined in mod-user-import, not mod-users. Moving issue to correct module. further edit: have also confirmed that this issue does not exist in mod-users. That is, creating and altering user records in mod-users does not fail when a department ID past the first ten are used. This bug seems confined to mod-user-import. |
| Comment by Kyle Felker [ 04/Feb/22 ] |
|
After looking at the code for a bit, Im pretty sure that Zak is right about what's causing this-when the module pulls the list of departments to check the import data against, it does not specify a limit-so by default, it only gets the first ten records. I also suspect (though I have not yet checked) that other parts of the code that validate things like patron groups have the same problem. I can see three potential solutions here, some of which Zak has already outlined: 1. Set a limit for the search that's super-high and hope that it's high enough to get all entries and low enough that it does not crash anything. Obviously, this is super-easy to implement. It's also hack-y and may lead to more problems in the future. 2. Examine the totalRecords field in the returned list of departments, and if it's higher than ten, do another call that sets the limit to that number so as to get them all. This will make the code more inefficient since it may have to do more than one call to the endpoint to get all records, and I've also heard that the counts in these result sets may not be reliable. 3. Rework or create a new endpoint in mod-users that will return all department records without needing a limiter. I believe Zak has already suggested this. Obviously, this is more work, and involves changing code in (potentially) two separate modules. If we are going to do this for departments, then we should also do it for other endpoints that are being queried, like patron groups, which makes this even more work. If time is a factor here (and it seems like it is) then I'd recommend fix #1, with us slating some issues to more permanently address this problem later. |
| Comment by Kyle Felker [ 04/Feb/22 ] |
|
I cannot answer your question without more details on the specific behavior you are referencing. As I pointed out earlier, this issue is not in the users module-user data import is located in it's own module. |
| Comment by Kyle Felker [ 04/Feb/22 ] |
|
okay, this is fun: the code that fetches patron groups is getting around this by setting an explicit limit of 2147483647 records. I laughed out loud when I saw it. |
| Comment by Erin Nettifee [ 04/Feb/22 ] |
|
Kyle Felker oh. oh no. |
| Comment by Erin Nettifee [ 04/Feb/22 ] |
|
Molly Driscoll the UI fetch for the department field on the user edit screen is querying with a limit of 10,000 so you can definitely have long lists of departments there. And the UI filter on the user search is the multiselect that's used in places like locations, so I think it works fine. I just put 11 departments on Snapshot and was able to see all of them there. So unless someone has recently changed that code, this may just be limited to the mod-user-import tool (which is one of the orphaned modules, without a primary dev team for a while) |
| Comment by Molly Driscoll [ 04/Feb/22 ] |
|
Erin Nettifee, thanks! I think it was changed Juniper to Kiwi because the library we discovered this with is on Juniper and the issue is there. I was multitasking so much that I didn't even think to test Bugfest. |
| Comment by Marc Johnson [ 07/Feb/22 ] |
|
Firstly, the Prokopovych team does not own this module. At the very least, I think it could be worth involving the folks that do in this conversation. This topic was discussed in the TC channel on Slack. There doesn't seem to be a conclusion to those conversations beyond asking developers to think about this stuff.
I agree with all of the concerns here.
I wouldn't trust the total records count for this logic. Instead, fetch pages until an empty page is fetched. This approach potentially makes the memory usage more manageable than a high arbitrary limit. We really don't have the tooling to make these decisions well in the technical space alone.
I think Zak Burke raised that this approach is highly dependent upon there being a stable expectation for the number of departments in an organisation. As usual, this is the kind of thing that we need both the product folks involved in (to figure out those expected department counts) and the technical folks (to have a consistent way of doing this across the project).
I agree the easiest fix (and the one often chose in other areas) is the larger arbitrary limit. Maybe we could add something that checks if the count of departments is the same as the limit and logs / includes a warning in the response that this likely means departments are missing due to how they are fetched. As we don't own this module, we include them in the decision to defer a different solution. |
| Comment by Debra Howell [ 07/Feb/22 ] |
|
From SUPPORT SIG: Stephanie Buck This ticket has been moved to Vega |
| Comment by Stephanie Buck [ 07/Feb/22 ] |
|
Debra Howell and all, mod-user-import belongs to Core Platform. I have reassigned. cc: Jakub Skoczen and Adam Dickmeiss |
| Comment by Molly Driscoll [ 08/Feb/22 ] |
|
I see this is now listed as closed, which is fantastic - thanks so much! May I ask if this will appear in a Kiwi hotfix or will the fix first be available in Lotus? |