Skip to content
This repository was archived by the owner on Sep 5, 2020. It is now read-only.

Comments

Restrict user actions#8

Open
djkiourtsis wants to merge 3 commits intomasterfrom
restrict_user_actions
Open

Restrict user actions#8
djkiourtsis wants to merge 3 commits intomasterfrom
restrict_user_actions

Conversation

@djkiourtsis
Copy link

Prevent non-staff users from performing staff actions on tickets.

This adds the is_staff status of the current user to ticket view
context.  This will be used for hiding actions from non-staff users.
Now that non-staff users have access to the ticket view, we want to
prevent them from using staff actions.
@djkiourtsis
Copy link
Author

});

{% if not is_staff %}
// If the user is not staff, they should not have access to all operations

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some what pedantic, but this comment won't show unless the block does, and then contextually it might not make sense. I think we could use {# comment #}. Maybe that was not the intent of the author. Just a consideration.

{% endifequal %}

{% if helpdesk_settings.HELPDESK_UPDATE_PUBLIC_DEFAULT %}
{% if helpdesk_settings.HELPDESK_UPDATE_PUBLIC_DEFAULT or not is_staff %}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel like this should be an AND. Whatever the setting is, but then still don't show it to non staffers.

Copy link

@cworth-gh cworth-gh Feb 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right, (that this should be "AND is staff" rather than "OR NOT is_staff" but I'm a little confused because I thought I saw this radio button present before the branch and then not present after testing with the branch. (But I may also not be thinking quite straight as I'm on little sleep at this point...)

So, tell me what you see in testing.

UPDATE: I should have expanded the context that github was showing me here. This wasn't an "if" statement around the "Is this update public?" block, this was around a hidden field forcing that value to True (and the "else" statement was around the "Is this update public?" block).

So the logic of the condition was fine.

Meanwhile, we don't actually want this "Is this update public?" block anywhere, so I've just added a commit to the sites branch that sets the HELPDESK_UPDATE_PUBLIC_DEFAULT setting to True to make that block go away.

@cworth-gh cworth-gh force-pushed the restrict_user_actions branch from 7ef5ac7 to 9305068 Compare February 8, 2018 23:20
@djkiourtsis djkiourtsis force-pushed the restrict_user_actions branch from 9305068 to 5605565 Compare February 20, 2018 21:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants