Skip to content
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

Sirenia docker image testing - unsolicited prompt to apply changes to contents when no changes to Visibility have been made #6537

Open
2 tasks
eporter23 opened this issue Dec 12, 2023 · 2 comments · May be fixed by #6800

Comments

@eporter23
Copy link
Contributor

eporter23 commented Dec 12, 2023

Descriptive summary

While testing the steps in RA_1.15 from the Hyrax testing worksheet, an Admin user making a minor metadata-only edit to a work they did not deposit can save their changes, but then gets a prompt to "Apply changes to contents?" even though visibility/access has not been changed.

Steps to reproduce the behavior in User Interface (UI)

  1. Login as an Admin user
  2. Go to Dashboard > Works > All Works
  3. Edit a work deposited by a different user OR a work you deposited
  4. Make a minor metadata-only edit somewhere in the Description tab (do not adjust Visibility)
  5. Save the changes
  6. See the prompt to "Apply changes to contents? You have changed the access level..."

Actual behavior (include screenshots if available)

Include what version of Hyrax relates to this issue (3.x, 4.x, main branch, etc.) if appropriate, and any relevant error messages/tracebacks if you're reporting a bug.

Sirenia/Fedora 6 docker image - Hyrax 5 rc2
Screen Shot 2023-12-12 at 1 22 15 PM

Acceptance Criteria/Expected Behavior

  • If an admin user edits a work they did not deposit and they do not adjust Visibility, this prompt should not appear
  • The changes to the work should save and the Admin user should be returned to the view work page

A successful metadata-only edit should be confirmed such as shown in dev.nurax:
Screen Shot 2023-12-12 at 1 29 02 PM

Rationale (for feature request only)

Provide the rationale or user story that describes "why" this issue should be addressed. Especially if this is a new feature or significant change to the existing implementation.

If no edits have been made to Visibility, users should not see this prompt.

Related work

Link to related issues or prior related work here.

@abelemlih
Copy link
Contributor

abelemlih commented May 17, 2024

@eporter23 @rjkati the underlying problem is in this method in app/controllers/concerns/hyrax/works_controller_behavior.rb:

def permissions_changed?
      @saved_permissions !=
        case curation_concern
        when ActiveFedora::Base
          curation_concern.permissions.map(&:to_hash)
        else
          Hyrax::AccessControl.for(resource: curation_concern).permissions
        end
end

when using Valkyrie, the array comparison between @saved_permissions and Hyrax::AccessControl.for(resource: curation_concern).permissions is always returning false even when permissions are exactly the same, because order in the two arrays is not always the same.

This is the fix I added for the application we are currently developing for reference:

def permissions_changed?
      # Hyrax 5.0.1 override
      # Sometimes the order of permissions is not the same between saved permissions and new permissions for Valkyrie 
      # I am replacing array comparison (which relies on order) with comparing array sizes and checking for existence of elements in both arrays
      
      case curation_concern
      when ActiveFedora::Base
        @saved_permissions != curation_concern.permissions.map(&:to_hash)
      else 
        new_permissions = Hyrax::AccessControl.for(resource: curation_concern).permissions
        saved_permissions_set = @saved_permissions.to_set
        new_permissions.size != @saved_permissions.size || new_permissions.any? { |e| !saved_permissions_set.include? e }
      end
end

@abelemlih abelemlih linked a pull request May 17, 2024 that will close this issue
@abelemlih
Copy link
Contributor

@eporter23 @rjkati I also added this PR: #6800

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants