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

Fixed #720: Restricted button to remove scheduled status to admin users #721

Merged
merged 4 commits into from
Apr 4, 2017

Conversation

aaboffill
Copy link

Fixes # by aaboffill

Changes proposed in this pull request:

  • Added restriction to show the button to remove Scheduled Status Change to the admin users only.

Status

  • READY
  • HOLD
  • WIP (Work-In-Progress)

How to verify this change

  • Login into the system with a not admin user.
  • Go to a client's view.
  • Schedule a status change by using the status button, at the top-right.
  • Go to the list and the button to remove Schedule Status Change won't be displayed.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.846% when pulling fb08749 on aaboffill:dev_720 into 2075ffb on savoirfairelinux:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 94.934% when pulling 61ed13c on aaboffill:dev_720 into 2075ffb on savoirfairelinux:dev.

@lingxiaoyang
Copy link
Contributor

@aaboffill I'm sorry, I forgot to tell you that we have been using "rules" for the permission control: see this change in PR #592.

Code references:
https://github.com/savoirfairelinux/sous-chef/blob/dev/src/sous_chef/rules.py
In template: https://github.com/savoirfairelinux/sous-chef/blob/dev/src/member/templates/client/list.html#L15
In view function: https://github.com/savoirfairelinux/sous-chef/blob/dev/src/member/views.py#L51

Apart from this, all seems good to me. Can you change ".is_staff" to "sous_chef.edit"?

@aaboffill
Copy link
Author

@lingxiaoyang I added your suggestions using the defined rules.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 94.86% when pulling 3aed260 on aaboffill:dev_720 into 2075ffb on savoirfairelinux:dev.

@@ -1449,8 +1450,10 @@ def get_success_url(self):
)


class ClientStatusSchedulerDeleteView(generic.DeleteView):
class ClientStatusSchedulerDeleteView(
PermissionRequiredMixin, generic.DeleteView):
Copy link
Author

Choose a reason for hiding this comment

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

@lingxiaoyang This is an example of the ugly python code when we have 79 characters as the line limit. It's my opinion!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it may be unavoidable in some cases, even with a higher limit. But 79 is not enough for a Django application, for sure.

@lingxiaoyang
Copy link
Contributor

Verified:
image
vs
image

@lingxiaoyang lingxiaoyang merged commit 3368470 into savoirfairelinux:dev Apr 4, 2017
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.

None yet

3 participants