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

[core] Fix incorrect message/error for instrument access #8284

Conversation

regisoc
Copy link
Contributor

@regisoc regisoc commented Dec 19, 2022

Brief summary of changes

An incorrect error message was displayed on the frontend when a user did not have the permission to access an instrument.
The code was test looping on ALL permissions, but it failed when trying to do so on only ONE permission (the loop went through the instrument name and permission instead of the actual instruments objects).
This PR fix the error message linked to permission listing.

Testing instructions (if applicable)

  1. Add a new user
  2. Give the user permissions through the db and config.xml file except one permission on one instrument (e.g. instrument "bmi", permission "data_entry")
  3. Enter the permission selected in 2. in the config.xml file (instrumentPermissions section)
  4. Go to Loris > Menu > Candidate > Access Profile
  5. Choose one PSCID (candidate) then one associated visit (visit label)
  6. In the "Behavioural Battery of Instruments", choose one instrument the user should not have access to according to the access rules
  7. This should print the 403 page, with "You do not have access to this page." message

Link(s) to related issue(s)

@regisoc regisoc requested a review from laemtl December 19, 2022 00:10
@ridz1208
Copy link
Collaborator

@regisoc If I'm understanding correctly the issue is that when only 1 instrument permission is set it is treated like a string instead of an array but when more are set you get an array. If that is the case I would prefer one of the following fixes instead of making 2 different usecases:

  1. fix the code generating the instrumentpermissions array and make sure it always returns an array even if there is a single one
  2. do an is_array() check and if it returns false, wrap it in an array and then default to going to the forloop.

Option 1 would be the better option but it MIGHT (should be verified) affect other configurations in which case it might be a risky change for a bugfix.

@driusan
Copy link
Collaborator

driusan commented Dec 20, 2022

The code generating the array is $config->getSetting which does have that behaviour, but can't be changed. The usual way to get that behaviour is to wrap it in a Utility::asArray call to ensure you're dealing with an array.

@regisoc
Copy link
Contributor Author

regisoc commented Jan 11, 2023

I changed for the wrapper Utility::asArray.

@driusan driusan merged commit 8a0b2c8 into aces:24.1-release Jan 12, 2023
@ridz1208 ridz1208 added this to the 24.1.2 milestone Feb 1, 2023
zaliqarosli pushed a commit to zaliqarosli/Loris that referenced this pull request Mar 6, 2023
An incorrect error message was displayed on the frontend when a user did not have the permission to access an instrument.
The code was test looping on ALL permissions, but it failed when trying to do so on only ONE permission (the loop went through the instrument name and permission instead of the actual instruments objects).

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

Successfully merging this pull request may close these issues.

3 participants