reindexing a record will clear acls and permission groups

[orangewolf]

Summary

When writing specs I noticed my records would be correct in the index but then they would get updated and suddenly they would loose their read_groups / edit_groups / edit_users / read_users. Also if you save an ACL list and then index the record again at any point it will loose the visibility setting.

Guidance for testing, such as acceptance criteria or new user interface behaviors:

• create a record in the console
• index it
• view the visibility and permisisons
• save the acl
• view the visibility and permissions
• index it again
• view the visibility and permisisons

@samvera/hyrax-code-reviewers

[no-reply]

i think if this is right, just resource.permission_manager should do the trick (it builds one exactly like the || path if it's not present, and the second branch should be unreachable as is).

[orangewolf]

👍

[dlpierce]

The change to PermissionIndexer is good, but the ACL#save change results in problems.

Currently, reindexing an unpersisted resource is going to result in blank fields for those supplied by AccessControl objects. This is because AccessControl.for (used in AccessControlList#access_control_model) always builds the object using custom_queries.find_access_control_for or makes a blank one if the query fails. That query will always fail with an unsaved resource (even if the AccessControl is persisted) due to find_inverse_references_by calling ensure_persisted.

[orangewolf]

The change to PermissionIndexer is good, but the ACL#save change results in problems.

Currently, reindexing an unpersisted resource is going to result in blank fields for those supplied by AccessControl objects. This is because AccessControl.for (used in AccessControlList#access_control_model) always builds the object using custom_queries.find_access_control_for or makes a blank one if the query fails. That query will always fail with an unsaved resource (even if the AccessControl is persisted) due to find_inverse_references_by calling ensure_persisted.

@dlpierce I'm not following you. Are you saying that previous to this change calling index on an unsaved model "works" but leaves the acl fields blank, but removing @change_set = nil would cause it to fali instead? It's been too long, but I'm pretty sure that both changes were required to keep from ending up with acls missing on every reindex. indexing shouldn't (in my opinion) mutate the objects and repeated indexing should be idempotent. Is there a whole new case we need to cover?

[jeremyf]

The change to PermissionIndexer is good, but the ACL#save change results in problems.
Currently, reindexing an unpersisted resource is going to result in blank fields for those supplied by AccessControl objects. This is because AccessControl.for (used in AccessControlList#access_control_model) always builds the object using custom_queries.find_access_control_for or makes a blank one if the query fails. That query will always fail with an unsaved resource (even if the AccessControl is persisted) due to find_inverse_references_by calling ensure_persisted.

@dlpierce I'm not following you. Are you saying that previous to this change calling index on an unsaved model "works" but leaves the acl fields blank, but removing @change_set = nil would cause it to fali instead? It's been too long, but I'm pretty sure that both changes were required to keep from ending up with acls missing on every reindex. indexing shouldn't (in my opinion) mutate the objects and repeated indexing should be idempotent. Is there a whole new case we need to cover?

I think my findings here: https://gist.github.com/jeremyf/90f6c6ae81f9c4596b666237d5da2931?permalink_comment_id=4846614#gistcomment-4846614 are likely what @dlpierce is referring to.

[orangewolf]

@dlpierce you were 100% right here. Jeremy and I fixed it a different way that I think gets us both wins.