Allowing for multiple collection curators, studies in multiple collections (SCP-3849, SCP-3867, SCP-3872) #1265
Conversation
Codecov Report
@@ Coverage Diff @@
## development #1265 +/- ##
===============================================
- Coverage 68.99% 68.97% -0.02%
===============================================
Files 241 242 +1
Lines 18814 18887 +73
Branches 950 950
===============================================
+ Hits 12980 13027 +47
- Misses 5665 5691 +26
Partials 169 169
|
There was a problem hiding this comment.
This is good stuff, taking our Collections to a new level of sophistication. I think you can simplify a lot fo this code by operating on the associations (e.g. BrandingGroup.users) rather than the id properties (e.g. BrandingGroup.user_ids). By using the associations, Rails+Mongoid will handle the reciprocal relationship bookkeeping for you.
| image_info = branding_group_config.dig('images') | ||
| # dynamically assign image files | ||
| image_info.each do |attribute_name, filename| | ||
| File.open(Rails.root.join('test', 'test_data', 'branding_groups', filename)) do |image_file| | ||
| branding_group.send("#{attribute_name}=", image_file) | ||
| # copy file to tmp directory to avoid CarrierWave deleting the original |
| if @branding_group.users.include?(current_user) | ||
| user_ids << current_user.id unless user_ids.include?(current_user.id) | ||
| end |
There was a problem hiding this comment.
consolidate the logic
| if @branding_group.users.include?(current_user) | |
| user_ids << current_user.id unless user_ids.include?(current_user.id) | |
| end | |
| if @branding_group.users.include?(current_user) && !user_ids.include?(current_user.id) | |
| user_ids << current_user.id | |
| end |
|
|
||
| class SearchControllerTest < ActionDispatch::IntegrationTest | ||
| include Devise::Test::IntegrationHelpers | ||
| include Requests::JsonHelpers | ||
| include Requests::HttpHelpers | ||
| include Minitest::Hooks | ||
| include ::TestInstrumentor |
There was a problem hiding this comment.
nice, thanks for updating this test
| BrandingGroup.all.each do |collection| | ||
| associations = collection_map[collection.id.to_s] | ||
| collection.update(studies: associations[:studies], users: associations[:users]) | ||
| end |
There was a problem hiding this comment.
Not worth changing now, but reading your comment, I realize this migration probably would have been cleaner with a migration class like we use for featureFlags
class StudyMigrator
include Mongoid::Document
store_in collection: 'studies'
belongs_to :branding_group, optional: true,
end
and
class BrandingGroupMigrator
include Mongoid::Document
store_in collection: 'branding_groups'
belongs_to :user
end
Then you could just do:
BrandingGroup.all.each |bg|
bg.update!(
users: [BrandingGroupMigrator.find(bg._id).user],
studies: StudyMigrator.where(branding_group_id: bg._id).to_a
)
end
|
I've verified this on staging. See here, here and here. I've also updated the testing instructions above to fill a small gap I stumbled over. Anyone on the SCP team can independently validate this by signing into the non-admin account [email protected] using instructions here. |
Currently, each collection may only have one "curator" (user who controls assigning studies to collections), and a study may only belong to one collection. Also, curators are not allowed to edit the collection themselves, and must go through an SCP admin to make any updates to the collection itself (like the tag line, or visual characteristics).
This update allows for collections to have multiple curators (who can now directly edit the collection), and for studies to belong to multiple collections. This is done by changing the associations from
has_many -> belongs_totohas_and_belongs_to_manyfor theStudy,BrandingGroup(collections), andUsermodels. This way, we can access all the studies for a collection directly from the collection itself, and vice versa. This update also includes a new integration test suite to cover all new functionality. In addition, a reversible data migration has been added to migrate from the existing association structure to the new one.There is also now a visual indication on the Collections page as to whether a collection has been published or not. This only shows if a user is signed in, and is a curator for at least one collection (otherwise, the only collections visible will all be published, and the indicator is not necessary). Example:
MANUAL TESTING
rails db:migrateSyntheticBrandingGroupPopulator.populate_allThis PR satisfies SCP-3849, SCP-3867, and SCP-3872.