-
Notifications
You must be signed in to change notification settings - Fork 2
DT-1662: Ensure single Library Card per User #2536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
+ " LEFT JOIN roles r ON r.role_id = ur.role_id " | ||
+ " WHERE LOWER(u.email) = LOWER(:email) " | ||
+ " AND r.role_id = :roleId") | ||
User findUserByEmailAndRoleId(@Bind("email") String email, @Bind("roleId") Integer roleId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only used in tests
// Some queries look for `user_role_id` while those that use a prefix look for `u_user_role_id` | ||
private UserRole mapUserRoleFromRowView(RowView rowView, Integer userId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, I don't think this method should be necessary. However, I found that the mapping wasn't working for cases where the prefix was ur_
and couldn't get that to work like I think it should.
User user = userService.findUserByEmail(authUser.getEmail()); | ||
List<LibraryCard> libraryCards = libraryCardService.findLibraryCardsByUserId(userId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug. When we remove DAAs from a library card, we need to remove them from the user passed into the method via userId
. This code will remove a DAA from the AuthUser
's library card.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this method also be checking that the SO is from the same institution as the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently not enabled in any environment so I want to tackle any explicit functional changes to DAA work as a distinct task. We will need to re-evaluate the entire feature before we do any further work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a ticket to remind ourselves: https://broadworkbench.atlassian.net/browse/DT-1704
# Conflicts: # src/test/java/org/broadinstitute/consent/http/db/LibraryCardDAOTest.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enforces that a user may have only a single library card by replacing all references to collections of library cards with a single library card field. Key changes include updating test cases to use setLibraryCard instead of setLibraryCards, modifying service methods in UserService and DataAccessRequestService to check for a single library card, and adjusting the DAO/mappers and associated YAML schema for consistency.
Reviewed Changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/test/java/org/broadinstitute/consent/http/service/DataAccessRequestServiceTest.java | Updated tests to replace empty libraryCards checks with a null check on libraryCard |
src/test/java/org/broadinstitute/consent/http/resources/DataAccessRequestResourceTest.java | Modified tests to use setLibraryCard in place of setLibraryCards |
src/main/resources/assets/schemas/User.yaml | Deprecated the collections field and added the libraryCard field |
src/main/java/org/broadinstitute/consent/http/service/UserService.java | Adjusted user creation and retrieval to assign and output a single library card for backwards compatibility |
(Other files) | Consistent update from list-based library cards to a single library card across DAOs, mappers, and resource classes |
Files not reviewed (2)
- src/main/resources/changelog-master.xml: Language not supported
- src/main/resources/changesets/changelog-consent-2025-23-unique-lc-user.xml: Language not supported
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a suggestion:
ld.daa_id as lc_daa_id | ||
FROM users u | ||
LEFT JOIN user_role ur ON ur.user_id = u.user_id | ||
LEFT JOIN roles r ON r.role_id = ur.role_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we pull this common section into something separate referenced by this method and the other one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fboulnois if we do that, won't we get complaints from SemGrep? Or are you suggesting we also change that configuration at this time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had to guess, it's why this class is being refactored in this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had to guess, it's why this class is being refactored in this way.
This is the primary reason why this is refactored, yes. The string concatenation approach has been problematic to maintain and triggers semgrep issues. I would like to see if we can come up with a better way of re-using large query blocks in some other way, but for now, this is the least bad option as I see it.
@@ -191,7 +207,7 @@ void testFindUsersWithLCsAndInstitution() { | |||
|
|||
User user2 = createUserWithInstitution(); | |||
int lcId2 = libraryCardDAO.insertLibraryCard(user2.getUserId(), | |||
user.getDisplayName(), user.getEmail(), user.getUserId(), new Date()); | |||
user2.getDisplayName(), user2.getEmail(), user2.getUserId(), new Date()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
@@ -56,34 +48,56 @@ public void accumulate(Map<Integer, User> map, RowView rowView) { | |||
} | |||
} | |||
} catch (MappingException e) { | |||
//Ignore institution mapping errors, possible for new users to not have an institution | |||
logDebug("Error adding Institution to User: %s".formatted(e.getMessage())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the case of the debug logs. I find them helpful for local development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rushtong - Do I understand correctly that everyone can read every DAA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@otchet-broad - yes, all DAAs are intentionally public.
((PSQLException) e.getCause()).getSQLState()); | ||
} | ||
User user2 = createUser(); | ||
// Test Unique on library_card.user_email - note that we're using the same email as user1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@@ -101,7 +101,7 @@ public User(String json) { | |||
// Nor do we need to retrieve the full institution object from user-provided data. | |||
JsonObject filteredUserJsonObject = filterFields( | |||
userJsonObject, | |||
Arrays.asList("createDate", "institution", "libraryCards")); | |||
Arrays.asList("createDate", "institution", "libraryCard")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also have to support libraryCards
until they're completely deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we do. This method constructs a User
object. Since User
doesn't have a libraryCards
field so if a user-provided string does contain libraryCards
, it will be ignored on the next line in gson.fromJson()
Addresses
https://broadworkbench.atlassian.net/browse/DT-1662
Summary
This PR enforces that a user can only have a single library card. There are a significant number of test changes to support this requirement. For backwards compatibility with the UI, we're populating the User object with a
libraryCard
and a list oflibraryCards
. When the UI is updated to match this, we can come back and remove the deprecated field.Notes
There will need to be a follow-on PR in the UI to deal with the user model describing an array of library cards. See https://github.com/DataBiosphere/duos-ui/blob/ee9e70640759c33bed9e6b9f8b5393f4692dea8b/src/types/model.ts#L51 for where we need to start there.
See these tickets for that work:
There are deletes for duplicate
user_id
anduser_email
rows. In Prod, there are no records affected. In Dev and Staging, there are 2 and 1 record respectively that will be removed.Testing
Tested the db update with a recent db download from
Tested the UI Admin/SO Library Card table functionality
Have you read CONTRIBUTING.md lately? If not, do that first.