Hide location action buttons unless user has permission to do the action #1268

laura-barluzzi · 2017-03-10T01:35:02Z

Proposed changes in this pull request

Fix bug #1233: only show the location action buttons to users who have permission to do the action.

Screenshot of location view for user without edit/delete permission:

Screenshot of location view for user with edit/delete permission:

When should this PR be merged

No dependencies.

Risks

None.

Follow up actions

None.

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly? It should have the migration label if a new migration is added.
Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature has the manual testing spreadsheet been updated with instructions for manual testing?

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

oliverroick

Hi @laura-barluzzi, some the tests are failing. Please make sure all tests pass before we can proceed with the review.

laura-barluzzi · 2017-03-10T23:18:20Z

Hi @oliverroick, I made some changes and now all the tests run on my machine.

clash99

Hi @laura-barluzzi - thanks for your work on this! It looks good from a ui perspective! Can you just update your branch to be current with the latest master branch?

laura-barluzzi · 2017-03-11T22:46:05Z

Hi @clash99, I've updated it. Let me know if there's something else to do.

oliverroick

Hi @laura-barluzzi, thank you for your contribution. It looks almost ok, there are two things I would like to ask you to change:

Check the permissions against the correct object (see my comment in the code).
You have only added tests for users who have all permissions. Could you also add tests that render the view with users who don't have permissions and thus should not see the buttons?

Thanks!

oliverroick · 2017-03-13T08:50:50Z

cadasta/organization/views/mixins.py

@@ -186,6 +186,10 @@ def get_context_data(self, *args, **kwargs):
        project = self.get_project()
        context['is_allowed_add_location'] = user.has_perm('spatial.create',
                                                           project)
+        context['is_allowed_update_location'] = user.has_perm('spatial.update',
+                                                              project)


Your checkin the permission against the wrong object here. For spatial.create project is fine, because the user wants to create a spatial unit for the project. For spatial.update and spatial.delete, you need to use the spatial unit instance from the view; the permission needs to be checked against itself and not its project.

laura-barluzzi · 2017-03-20T03:46:00Z

Hi @oliverroick, I changed the logic so that the permission is checked against the location object instead of the project, but I struggle to figure out how to update the tests. Can you please guide me on this? Cheers

oliverroick · 2017-03-20T15:11:04Z

Hey @laura-barluzzi,

If you look at this part; this is where permissions are assigned to a user. In a default test the user has these permissions, so the buttons are shown.

You now need to add a test for the case when the buttons are not shown. In that test, you need to:

Create policies that explicitly deny permission to edit and delete the location.
Assign the policies to a users.
Get a rendered response using that user.
And evaluate if the site was rendereded correctly.

You can create policies that deny permissions by copying the clauses that I linked above and add another clause that looks like this:

{
    'effect': 'deny',
    'object': ['spatial/*/*/*'],
    'action': ['spatial.update', 'spatial.delete']
},

Combined with the other policies the user will be able to view all locations but not edit or delete any of them.

If you look at this test case, you'll see an example how to use a specific user with a request (for step 3) and how you can overwrite specific items in the template context for the rendered expected response to make sure the buttons are not rendered (for step 4).

Note: We're using django-skivvy to setup test cases for views. Each test class defines a standard template context for expected response for all the tests (see here for reference). And you can overwrite selected items using render_content.

laura-barluzzi · 2017-03-21T21:09:54Z

Hi @oliverroick, thank you for telling me how to add a new test. Before adding the new tests, I'd like to address something I'm confused about and I was hoping you could help me figure this out.

Currently some of the existing tests fail, e.g. https://travis-ci.org/Cadasta/cadasta-platform/jobs/212860743#L955-L975

Currently all the exiting test template contexts render the edit/delete location buttons e.g. https://github.com/Cadasta/cadasta-platform/pull/1268/files#diff-d2f99481b1c4d834b7e971c17527a28cR590

And by default, the user has all permissions on the spatial object: https://github.com/laura-barluzzi/cadasta-platform/blob/bugfix/%231233/cadasta/spatial/tests/test_views_default.py#L42-L44

So doesn't that mean that the buttons should get rendered and the tests should pass? I'm confused what's going on here. Why the tests are not passing?

oliverroick · 2017-03-22T18:54:10Z

@laura-barluzzi

You have only added the permissions check to LocationDetail but not to any of the other views. If you look at the tests that are failing you'll see which of the views you need to change.

It would be a good idea to move the functionality to work out the permissions into a mixin and apply that to all the views that require it. Otherwise you'd be replicating a lot of code.

Good job on writing the tests so thoroughly!

laura-barluzzi · 2017-03-23T19:50:08Z

Hi @oliverroick, I figured out how to make all the tests pass and I pushed this commit. Next I'll add the test for cases without buttons.

laura-barluzzi · 2017-03-23T23:52:43Z

Hi @oliverroick I added the test with the denied edit/delete permission. Let me know what you think and if there's something else to do before merging.

oliverroick

Hi @laura-barluzzi, you're almost there. There's a small bug in the template that you still need to fix. See the comment in the code

oliverroick · 2017-03-29T15:41:34Z

cadasta/templates/spatial/location_detail.html

@@ -12,8 +12,12 @@
      <h2 class="short"><span>{% trans "Location" %} </span>{{ location.name }}</h2>
      <div class="top-btn pull-right">
        <!-- Action buttons -->
-        <a class="btn btn-default btn-action btn-sm" href="{% url 'locations:edit' object.organization.slug object.slug location.id %}" title="{% trans 'Edit location' %}" aria-label="{% trans 'Edit location' %}"><span class="glyphicon glyphicon-pencil"></span></a>
-        <a class="btn btn-default btn-action btn-sm" href="{% url 'locations:delete' object.organization.slug object.slug location.id %}" title="{% trans 'Delete location' %}" aria-label="{% trans 'Delete location' %}"><span class="glyphicon glyphicon-trash"></span></a>
+        {% if is_allowed_update_location %}


The edit button is never shown because you're using the wrong template context name here.

oliverroick

Good to go from my end. Thanks!

…ete permissions

laura-barluzzi mentioned this pull request Mar 10, 2017

Hide location edit and delete buttons for project users #1233

Closed

clash99 self-requested a review March 10, 2017 14:49

oliverroick requested changes Mar 10, 2017

View reviewed changes

oliverroick assigned oliverroick and laura-barluzzi Mar 11, 2017

oliverroick assigned clash99 and unassigned laura-barluzzi Mar 11, 2017

clash99 suggested changes Mar 11, 2017

View reviewed changes

oliverroick requested changes Mar 13, 2017

View reviewed changes

laura-barluzzi mentioned this pull request Mar 24, 2017

Project User Can See Forms but Not Download Them in ODK #1239

Open

amplifi added PR: needs review PR: needs testing labels Mar 28, 2017

oliverroick requested changes Mar 29, 2017

View reviewed changes

oliverroick approved these changes Apr 3, 2017

View reviewed changes

clash99 approved these changes Apr 3, 2017

View reviewed changes

amplifi added PR: approved and removed PR: needs review labels Apr 3, 2017

laura-barluzzi added 7 commits April 3, 2017 20:56

Hide location action buttons unless user has permission to do the action

daea6fd

Hide location action buttons unless user has permission to do the action

d319d2e

check update/delete permissions against location, not project

ee59a27

add default tests only in template with location details

ee6d823

add test for not displaying buttons when users is denied edit and del…

2a48031

…ete permissions

Make flake8 happy

0f1cc3e

fix text in template to show edit button when user has edit permission

b88a339

amplifi force-pushed the bugfix/#1233 branch from 054e791 to b88a339 Compare April 3, 2017 13:57

amplifi merged commit 38a85ab into Cadasta:master Apr 3, 2017

laura-barluzzi mentioned this pull request Aug 22, 2017

Fixes #1693: displays edit and delete button in party detail page only if has permission #1735

Merged

46 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide location action buttons unless user has permission to do the action #1268

Hide location action buttons unless user has permission to do the action #1268

laura-barluzzi commented Mar 10, 2017

oliverroick left a comment

laura-barluzzi commented Mar 10, 2017

clash99 left a comment

laura-barluzzi commented Mar 11, 2017

oliverroick left a comment

oliverroick Mar 13, 2017

laura-barluzzi commented Mar 20, 2017

oliverroick commented Mar 20, 2017

laura-barluzzi commented Mar 21, 2017

oliverroick commented Mar 22, 2017

laura-barluzzi commented Mar 23, 2017

laura-barluzzi commented Mar 23, 2017

oliverroick left a comment

oliverroick Mar 29, 2017

oliverroick left a comment

Hide location action buttons unless user has permission to do the action #1268

Hide location action buttons unless user has permission to do the action #1268

Conversation

laura-barluzzi commented Mar 10, 2017

Proposed changes in this pull request

When should this PR be merged

Risks

Follow up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Documentation

oliverroick left a comment

Choose a reason for hiding this comment

laura-barluzzi commented Mar 10, 2017

clash99 left a comment

Choose a reason for hiding this comment

laura-barluzzi commented Mar 11, 2017

oliverroick left a comment

Choose a reason for hiding this comment

oliverroick Mar 13, 2017

Choose a reason for hiding this comment

laura-barluzzi commented Mar 20, 2017

oliverroick commented Mar 20, 2017

laura-barluzzi commented Mar 21, 2017

oliverroick commented Mar 22, 2017

laura-barluzzi commented Mar 23, 2017

laura-barluzzi commented Mar 23, 2017

oliverroick left a comment

Choose a reason for hiding this comment

oliverroick Mar 29, 2017

Choose a reason for hiding this comment

oliverroick left a comment

Choose a reason for hiding this comment