Added read_only role

[umair-nasir14]

The read_only role can only see the response list. For now, it can also assign reviewers.

[avishkar58]

Hey Muhammad, Thanks for the PR!
I have a question on the overall design - I think it makes total sense to have an event role for "read_only" which gives read-only access to a particular event, which you have. But what I'm not sure about is the reason for adding a global is_read_only flag like how we have the global is_admin - I can't think of a use-case that would require global read-only access - is there one for AIMS perhaps that I'm missing?
Thanks!

[umair-nasir14]

Hi Avishkar!

Thanks for the review!

So from my understanding, AIMS have many directors who require read-only access while viewing application responses. I thought it would be fine to have a global flag which can cover all the use cases. Don't know if this makes sense.

Thanks again!

[avishkar58]

Thanks for the explanation! So we generally try to avoid adding global flags to the users, because this is something that affects the entire system for all organisations and globals have a way of polluting the codebase. You'll notice that in the user model we have a lot of fields that are no longer used because those globals which we introduced initially reduced the flexibility of the system and so we had to deprecate them. In this case I don't think the motivation to add a "read-only" global is strong enough, so lets please remove that and stick to the event role.

Then for the event role, I think it would be better to make the role more fine-grained. From what I understand, the requirement here is to give read-only access to the responses, so lets make the role exactly that: "response-reader". I see in your description that the current "read_only" role also gives the ability to assign reviewers, which is a write action and therefore very confusing. Lets introduce another role for that, "review-manager" or something like that. We can add as many event roles as we like, so I'd prefer to keep those more fine-grained and intuitive as to what they mean. (For now, imagine an admin who isn't familiar with the code, seeing this event role "read-only", I think in its current state the admin would not be expecting it to do what it currently does.)

[umair-nasir14]

Thanks for the detailed review.

Understood, makes perfect sense! I will get back to you when it is done.