Make user special access case insensitive#6764
Conversation
cjcolvar
left a comment
There was a problem hiding this comment.
I was thinking about this and I'm wondering if it might make more sense to add before_save callbacks on the models instead of doing the downcasing the next layer up in the controller/jobs. Either way I think we'll probably need a migration to downcase existing read_users which could be expensive. We might be able to do a solr query for uppercase characters to try and avoid migrating every collection and media object in the repository. Alternatively we could not change how they're stored and try to find a way to downcase before querying. But that seems like it would probably be tricky and involve overrides. Thoughts?
bb787d2 to
f245f44
Compare
cjcolvar
left a comment
There was a problem hiding this comment.
This is looking great!
Unfortunately, I do have a couple thoughts which could expand the scope of this PR more so see if you think it would be best to handle it here or in another PR or another time (which could be never 😆):
So if usernames in special access are getting downcased then shouldn't we handle usernames across the application as downcased? If so, then unit and collection roles as well as Admin::Group members should all be downcased. (There might possibly be other places where usernames are getting used in playlists/timelines?) And if all of the string references to users are being downcased then we should probably downcase the username/email in the user model too. If those changes are made in before_save callbacks on the models then we probably don't need the downcases throughout the application. We would have to migrate more objects but the ActiveRecord user objects should be fast.
|
I think there is possibly too much to that discussion for the comment section here. Let's try to meet up synchronously sometime this week and hash all that out. |
Related issue: #6742
Co-authored-by: Chris Colvard <chris.colvard@gmail.com> The `read_users` field on the media object is not an active fedora property so `read_users.changed?` does not exist, so I think that the before_save callback has to run everyt time instead of being proc-ed to only when the field has been updated.
Co-authored-by: Chris Colvard <chris.colvard@gmail.com> Instead of relying on an arbitray hardcoded number of rows and manually running the task multiple times if it does not catch everything we should gather the count of affected records and use a while loop to iterate. This also adds logging to the console so users can track how many records are found, how many are successfully migrated, and what errors are encountered for records that failed.
5923313 to
7646170
Compare
Related issue: #6742