User#can_edit? fix

[TunrayoIlawole]

What this PR does

This PR addresses #5948 by ensuring users with multiple roles are correctly assigned editing permissions.

More details on what was done

• Updated User#role method to properly handle cases where a user has multiple roles in a course.
• The method now checks if the user has any editing role and returns the appropriate role.
• Added unit test to verify that users with both campus volunteer (non-editing role) and instructor roles are correctly recognized as having editing privileges.

[ragesoss]

I think this would be better solved by implementing a different method to check for which roles a user has for a course. The User#role method is designed (and named) to return a single user role, but for can_edit? we want to consider all the roles a user has for a course, and check whether any of them are editing roles.

Maybe something like this would work in place of the call to User#role: return true if CoursesUsers.exists?(course: course, user: self, role: EDITING_ROLES)

[TunrayoIlawole]

Thank you @ragesoss . Noted, i will make the update

[TunrayoIlawole]

Hi, @ragesoss . I created the new method to check whether any of the roles a user has for a course is an editing role and called that in can_edit

[ragesoss]

Please check the build log; it looks like this has broken something.

[TunrayoIlawole]

Hi @ragesoss, I noticed that in cases where the course is nill, the call to User#role returns an Instructor role which ultimately returns a true value, bypassing the need to check User#campaign_organizer?

For my new method, should i follow a similar approach and return true if the course is nill, since an instructor role is considered an editing role? Or would it be more appropriate to implement a check that returns early in User#campaign_organizer?

I’d appreciate your advice on the best way to handle this case to ensure it aligns with the project's standards. Thank you.

[ragesoss]

The behavior of returning the instructor role for cases where the course is nil is confusing, and I'm not exactly sure how we use that. Based on the comment, it seems to be related to the course creation process, before the CoursesUsers record for the instructor has been created, because the course itself as not been saved in the database. But the best thing to do might be to identify exactly where we use that behavior (for example, by throwing an error instead of returning the instructor role, and seeing which tests fail) and handle that differently, so that role can be a little more intuitive.

[TunrayoIlawole]

Hi, @ragesoss, thank you for your feedback. The behaviour is not really relied on, except in determining whether a user has an editing role.

The main issue with assigning an Instructor role to a user that is not associated with any course (i.e course is nil) is that it automatically gives the user an editing role. This prevented the error-prone User#campaign_organizer?, which currently does not handle nil values for course and therefore fails, from being called.

This is the code that is failing. course.campaigns throws an error because course is nil

  def campaign_organizer?(course)
    CampaignsUsers.exists?(user: self, campaign: course.campaigns,
                           role: CampaignsUsers::Roles::ORGANIZER_ROLE)
  end

The pipeline is failing because User#campaign_organizer? is now being called as a result of the changes i made to the User#can_edit code to no longer rely on User#role for determining whether a user has an editing role or not.

I propose:

• Returning false in User#campaign_organizer? if the course is nill
• Assigning a default, non-editing role to a user that is not associated with a course or raising an error . Either of these methods will require significant changes to the logic and tests.

Please let me know what you think, @ragesoss

[ragesoss]

Which tests fail when you throw an error from the course.nil? check?

[TunrayoIlawole]

Hi, @ragesoss , the following tests fail when i throw an error from the nil check:

• User#role grants instructor permission for a user creating a new course
• AssignmentsController PATCH #update_status when a status param is provided renders a 200
• AssignmentsController PATCH #update_status when no status param is provided renders a 422
• AssignmentsController PATCH #update_sanbox_url updating sandbox url with valid urls update sandbox url successfully with example 1
• AssignmentsController PATCH #update_sanbox_url updating sandbox url with valid urls update sandbox url successfully with example 2
• AssignmentsController PATCH #update_sanbox_url updating sandbox url with valid format but belongs to different wiki does not update url and send response with status: unprocessable entity
• AssignmentsController PATCH #update_sanbox_url updating sandbox url with invalid url format does not update url and send response with status: bad request example 1
• AssignmentsController PATCH #update_sanbox_url updating sandbox url with invalid url format does not update url and send response with status: bad request example 2

There are other tests that fail for both implementations(i.e with error thrown and with no error thrown), such as: updating locale via URL updates the locale and redirects to the home page

and other errors from Capybara

[ragesoss]

Hmm... that's weird. I would not expect course to be nil in those AssignmentsController tests, as creating the course record is part of the setup. Did you configure it to only throw an error when course is nil, or every time that line is reached?

[TunrayoIlawole]

Hi @ragesoss , sorry i should have clarified. The AssignmentsController tests are failing because User#campaign_organizer? is now being called in the code. It was not called before because User#can_edit? automatically returned true as a result of the instructor role being returned from User#role.

So i belive the only test that is directly failing as a result of the error that i threw in User#role is User#role grants instructor permission for a user creating a new course

[ragesoss]

Okay. I'm still not clear on why the assignment tests rely on the instructor role behavior. Is it because the test setup is incorrect?

[TunrayoIlawole]

They do not rely on the instructor role @ragesoss . Apologies for the confusion. They are failing because of a missing nil check in User#campaign_organizer. The reason why I kept referencing the AssignmentsController tests is because I wanted to give some context as to why they were not failing with the previous implementation.

[ragesoss]

Okay. So it sounds like, as suspected, the only test that directly relies on the instructor role-for-nil-course behavior is the one related to creating a new course, but that behavior was also masking either a bug with the campaign_organizer check, or a bug with test setup related to it. Before adding a new nil check, we want to understand whether any actual valid requests can have a nil value, or whether it's just something that is missing from the test setup but in actual usage it could never be nil.

The other thing to look into is how role gets called when creating a new course, as we may want to find a different way of handling it.

[TunrayoIlawole]

Hi @ragesoss , thank you for your feedback.

From my investigation, i can see that User#role is only called in the application to verify edit permissions and confirm access to a course.

When a course is created by a user, that user is automatically assigned an instructor role. Also, the course is returned whether it was successfully persisted in the database or not. I believe this is the reason why User#role was set to return an instructor role for an empty course.

So the idea is whenever User#role (User = current user i.e the user that created the course) is called with a nil course value, it automatically returns the instructor role so as to avoid not getting any result here in case the course has not been saved to the database.

But technically, i do not think any actual valid requests can have a nil course value since a course object is returned whether the course was successfully persisted or not, so User#role will always be called with a course object.

role does not get called when creating a new course.

Also, i think the description of this test might be misleading because User#role only determines the role of a user in a given course, it has nothing to do with course creation.

Please let me know what you think.

[ragesoss]

Interesting. So it sounds like we should probably remove that behavior altogether, for handling a nil course.

I think implementation of User#role is very confusing, as it is really more about permissions than roles. Since it is only called from a few places, I think the best thing would be to refactor it from #role to #course_roles, make that method simply return an array roles the user has in the course, and move the logic for handling permissions for admins and campaign organizers over to the methods that are explicitly about permission (User#can_edit? and maybe a new User#can_view? for use in protect_privacy.)

[TunrayoIlawole]

Noted @ragesoss , I will work on that.

The AssignmentsController tests are also failing because for some reason, when User#can_edit is called in AssignmentsController#check_permissions, an empty course is passed. I am looking into that

[TunrayoIlawole]

Hi @ragesoss , I discovered that most of the AssignmentControllers tests that are failing are failing due to incomplete request parameters being passed.

But i am still having an issue with two of the tests: 1 & 2 and it has to do with not calling set_course before executing update_status - see code

Is there a reason for that behaviour?

[ragesoss]

The JS action that hits the update_status endpoint does not include any course identifier, so I think set_course would fail. This comes from updateAssignmentStatus in the assignment_actions.js file.

[ragesoss]

Probably the right approach is to set the course via the assignment (as with #destroy) before performing the permissions check.

[TunrayoIlawole]

Hi @ragesoss , please can i push the fix for both User#role and the AssignmentControllers tests to this same PR?

[ragesoss]

@TunrayoIlawole yes

[TunrayoIlawole]

Hi @ragesoss , here are all the changes I made to the code:

• created a method User#editing_role? to check if a user has any editing role in a course
• created a method to return an array of all the courses a user has in a course
• created User#can_view method
• updated User#nonvisitor? to return true if a user has any role other than a visitor role
• updated User#can_see_real_names? to return true if a user has any role that is included in the REAL_NAME_ROLES object
• created tests for all new methods
• updated request_params in some assignment controller tests to include the course_slug so that the course can be set correctly via AssignmentsController#set_course
• set course before checking permissions in AssignmentsController#update_status and AssignmentsController#claim

[TunrayoIlawole]

Hi @ragesoss, thank you for the feedback. For methods where User#role is used for non-permission reasons such as this and this , is it okay to return the highest role as was done in the previous implementation?

Can we have a new User#role method that takes the array from User#course_roles and returns the highest role? And in the case where a user is not associated with a course (i.e a visitor) and User#course_roles returns an empty array, we automatically return the visitor role?

Please let me know what you think.

[ragesoss]

I think that strategy makes sense. I would suggest naming these methods very clearly: have #user_roles return all roles, and have #highest_role return the highest role, and any additional functionality required for a specific reason should have its own method building on top of those (or implement additional logic independently).

My priority is to make the public methods on this widely-used class as simple and self-explanatory as possible.

[ragesoss]

I suspect that these two failing tests are related to the changes; they seem like tests that would be affected by 'can edit' checks.

[TunrayoIlawole]

Thank you, @ragesoss . Update made