Cleanup and bug fixes for cohort and project group Access Controls #5087

eanders · 2025-01-24T21:03:06Z

Merging this PR

use the squash-merge strategy for PRs targeting a release-X branch

Description

Updates entity access concern to correctly re-use collections and user groups if the names of project groups or cohorts change
Updates the edit UI for cohorts and project groups to hide irrelevant access fields
Updates the list views for access controls, collections, and user groups, to ignore items that aren't editable and are maintained by the system elsewhere (on cohorts or project groups)
Adds tests to confirm changing entity name doesn't break the underlying connections
Adds documentation to various methods
Migrates existing system collections and user groups to have a direct connection to their source

Type of change

Bug fix
Documentation

Checklist before requesting review

I have performed a self-review of my code
I have run the code that is being changed under ideal conditions, and it doesn't fail
I have updated the documentation (or not applicable)
I have added spec tests (or not applicable)
I have provided testing instructions in this PR or the related issue (or not applicable)

//: # NOTE: system tests may fail if there is no branch on the hmis-frontend that matches the Source or Target branch of this PR. This is expected

…t page

…and cohorts maintain the same; Hide various system items from the lists of collections and user groups; Hide access fields on cohort and project group pages unless relevant

ttoomey

Looks good! I left a few comments and suggestions

ttoomey · 2025-01-24T23:46:19Z

app/views/cohorts/edit.haml

+        -# TODO: START_ACL remove after ACL transition
+        -# if everyone is using Access Controls, hide this section, but keep it in the form
+        -# so we don't lose data during the change over
+        - visibility_class = if User.all_using_acls? then 'd-none' else '' end


This syntax is legal but it seems pretty unusual to me; I don't know that I've seen this in other code bases. I have a feeling that it's best to stay within the conventions of the language particularly for such a common type of expression

Would a ternary be okay since it's a short line?

Suggested change

- visibility_class = if User.all_using_acls? then 'd-none' else '' end

- visibility_class = User.all_using_acls? ? 'd-none' : nil

or

Suggested change

- visibility_class = if User.all_using_acls? then 'd-none' else '' end

- visibility_class = 'd-none' if User.all_using_acls?

ttoomey · 2025-01-25T00:00:00Z

app/views/cohorts/edit.haml

+        -# if everyone is using Access Controls, hide this section, but keep it in the form
+        -# so we don't lose data during the change over
+        - visibility_class = if User.all_using_acls? then 'd-none' else '' end
+        - hint_text = if User.anyone_using_acls? then 'Specified user and members of specified groups will have access to this cohort. This method of access is being deprecated, to be replaced with the fields below.' else 'Specified user and members of specified groups will have access to this cohort.' end


similar concern, can we avoid this 'if..else..end' construction?

Suggested change

- hint_text = if User.anyone_using_acls? then 'Specified user and members of specified groups will have access to this cohort. This method of access is being deprecated, to be replaced with the fields below.' else 'Specified user and members of specified groups will have access to this cohort.' end

- hint_text = 'Specified user and members of specified groups will have access to this cohort.'

- hint_text += ' This method of access is being deprecated, to be replaced with the fields below.' if User.anyone_using_acls?

ttoomey · 2025-01-25T00:07:10Z

app/views/cohorts/edit.haml

+        - visibility_class = if User.anyone_using_acls? then '' else 'd-none' end
+        %div{ class: visibility_class }


can we avoid reusing the visibility_class var from 23? Maybe we don't need a variable?

Suggested change

- visibility_class = if User.anyone_using_acls? then '' else 'd-none' end

%div{ class: visibility_class }

%div{ class: (User.anyone_using_acls? && 'd-none') }

ttoomey · 2025-01-25T00:14:19Z

app/controllers/admin/access_controls_controller.rb

@@ -13,7 +13,7 @@ class Admin::AccessControlsController < ApplicationController

  def index
    @access_controls = access_control_scope.
-      joins(:role, :collection).
+      left_outer_joins(:collection, :role, :user_group).


do we need these joins? It looks like the editable scope is doing that already

ttoomey · 2025-01-25T00:16:21Z

app/models/access_control.rb

@@ -36,6 +37,16 @@ class AccessControl < ApplicationRecord
    joins(:collection).merge(Collection.system)
  end

+  # Where not ALL role, collection, user group are system
+  scope :editable, -> do


I wonder if there's a more descriptive name than editable. Maybe not_system_managed or user_managed

ttoomey · 2025-01-25T00:37:38Z

app/models/concerns/entity_access.rb

      collection.set_viewables(entity_relation_type => [id])
+      collection.update(name: name) # ensure the collection name still matches


(same throughout this file)

Suggested change

collection.update(name: name) # ensure the collection name still matches

collection.update!(name: name) # ensure the collection name still matches

app/models/concerns/entity_access.rb

ttoomey · 2025-01-25T00:54:19Z

app/models/concerns/entity_access.rb

+  end
+
+  # NOTE: this only needs to be run once, and only if the installation was created
+  # prior to release-151


maybe we should add a todo-or-die to remove dead code once everyone has migrated?

Suggested change

# prior to release-151

# prior to release-151

TodoOrDie('Remove fix_entity_associations', by: '2025-04-01')

Or alternatively move the method into the migration where we will purge it eventually

ttoomey · 2025-01-25T01:01:12Z

db/migrate/20250124170335_add_source_to_collections_and_user_groups.rb

+    add_reference :collections, :source, polymorphic: true, index: { algorithm: :concurrently }
+    add_reference :user_groups, :source, polymorphic: true, index: { algorithm: :concurrently }


collections and user groups are pretty small tables, we probably could just build the index non-concurrently and stay safe with our ddl tx?

I also wonder if we should make these indexes unique. There shouldn't be two collections referencing the same source entity right?

Suggested change

add_reference :collections, :source, polymorphic: true, index: { algorithm: :concurrently }

add_reference :user_groups, :source, polymorphic: true, index: { algorithm: :concurrently }

safety_assured do

add_reference :collections, :source, polymorphic: true, index: { unique: :true }

add_reference :user_groups, :source, polymorphic: true, index: { unique: :true }

end

ttoomey · 2025-01-25T01:57:16Z

app/models/concerns/entity_access.rb

+      # Find the last collection that would have been created under the old mechanism
+      collection = Collection.system.where(name: name).
+        order(id: :desc).
+        first_or_create


Are these normally created on-demand? Do we expect every project group or cohort to have a collection/usergroup? If not maybe we don't need to make new records unless the collection/user group already exists.

Also probably should use bang variant:

Suggested change

first_or_create

first_or_create!

eanders added 4 commits January 24, 2025 08:50

Only show access controls that are editable on the access control lis…

5c1bb5d

…t page

Enforce system collections and system user groups for project groups …

d8507f4

…and cohorts maintain the same; Hide various system items from the lists of collections and user groups; Hide access fields on cohort and project group pages unless relevant

Revert sql; remove unneeded changes

0d1e901

rubocop

7a51d32

eanders requested review from dtgreiner and ttoomey January 24, 2025 21:17

ttoomey approved these changes Jan 25, 2025

View reviewed changes

eanders added 2 commits January 25, 2025 14:13

Review feedback

cdf7e21

Review feedback

34c7792

dtgreiner approved these changes Jan 28, 2025

View reviewed changes

eanders merged commit aef0bf0 into release-150 Jan 29, 2025
54 checks passed

eanders deleted the ea/system-collections branch January 29, 2025 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup and bug fixes for cohort and project group Access Controls #5087

Cleanup and bug fixes for cohort and project group Access Controls #5087

eanders commented Jan 24, 2025 •

edited

Loading

ttoomey left a comment

ttoomey Jan 24, 2025

ttoomey Jan 25, 2025

ttoomey Jan 25, 2025

ttoomey Jan 25, 2025

ttoomey Jan 25, 2025

ttoomey Jan 25, 2025

ttoomey Jan 25, 2025

ttoomey Jan 25, 2025

ttoomey Jan 25, 2025

	- visibility_class = if User.all_using_acls? then 'd-none' else '' end
	- visibility_class = User.all_using_acls? ? 'd-none' : nil

	- visibility_class = if User.all_using_acls? then 'd-none' else '' end
	- visibility_class = 'd-none' if User.all_using_acls?

	- hint_text = if User.anyone_using_acls? then 'Specified user and members of specified groups will have access to this cohort. This method of access is being deprecated, to be replaced with the fields below.' else 'Specified user and members of specified groups will have access to this cohort.' end
	- hint_text = 'Specified user and members of specified groups will have access to this cohort.'
	- hint_text += ' This method of access is being deprecated, to be replaced with the fields below.' if User.anyone_using_acls?

		- visibility_class = if User.anyone_using_acls? then '' else 'd-none' end
		%div{ class: visibility_class }

	- visibility_class = if User.anyone_using_acls? then '' else 'd-none' end
	%div{ class: visibility_class }
	%div{ class: (User.anyone_using_acls? && 'd-none') }

		collection.set_viewables(entity_relation_type => [id])
		collection.update(name: name) # ensure the collection name still matches

	collection.update(name: name) # ensure the collection name still matches
	collection.update!(name: name) # ensure the collection name still matches

	# prior to release-151
	# prior to release-151
	TodoOrDie('Remove fix_entity_associations', by: '2025-04-01')

		add_reference :collections, :source, polymorphic: true, index: { algorithm: :concurrently }
		add_reference :user_groups, :source, polymorphic: true, index: { algorithm: :concurrently }

-    add_reference :collections, :source, polymorphic: true, index: { algorithm: :concurrently }
-    add_reference :user_groups, :source, polymorphic: true, index: { algorithm: :concurrently }
+   safety_assured do
+      add_reference :collections, :source, polymorphic: true, index: { unique: :true }
+      add_reference :user_groups, :source, polymorphic: true, index: { unique: :true }
+    end

Cleanup and bug fixes for cohort and project group Access Controls #5087

Cleanup and bug fixes for cohort and project group Access Controls #5087

Conversation

eanders commented Jan 24, 2025 • edited Loading

Merging this PR

Description

Type of change

Checklist before requesting review

ttoomey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eanders commented Jan 24, 2025 •

edited

Loading