Fix for https://github.com/Cadasta/cadasta-platform/issues/682 #702
Conversation
|
Hi @manoramahp, thank you for creating this PR. Unfortunately, this PR cannot be merged in its current state because it is failing the continuous integration test in Travis. If you click on "Details" below to the right where it says "The Travis CI build failed", you will see that your PR is failing 8 unit tests. These tests need to pass before we can merge the PR. I suggest seeing what tests failed and then investigating what needs to be modified, either in the test or in the Django code. |
| @@ -47,6 +47,7 @@ | |||
| </ul> | |||
| </div> | |||
| {% endif %} | |||
| {% if is_allowed_add_resource %} | |||
There was a problem hiding this comment.
Based on where you placed this condition, it seems you are also hiding the "Add location" option. Please take note that it may be possible that a user does not have the add resource permission, but still have the add location permission. Your modification here hides the add location option even if the user is allowed to do so.
| @@ -106,11 +106,13 @@ | |||
| <h3 style="text-transform: capitalize;">{% trans "Congratulations!" %}</h3> | |||
| <p>{% trans "You have successfully created your project. You're now ready to add your first location." %}</p> | |||
| {% endif %} | |||
| {% if allow_add_resource %} | |||
There was a problem hiding this comment.
This will be confusing. The context variable you added is allow_add_resource, but what you are conditionally hiding is the "Add a location" button. Please take note that adding resources is different from adding locations and the user may have differing permissions for each of these actions.
There was a problem hiding this comment.
@seav
If the user doesn't have add location permission, can he have add resource permission?
| @@ -121,9 +121,28 @@ def is_administrator(self): | |||
| self.is_admin = True | |||
| return self.is_admin | |||
|
|
|||
| @property | |||
| def is_allowed_add_resource(self): | |||
There was a problem hiding this comment.
In the future, we will allow the project manager to set fine-grained permissions. This means that whether a user is allowed to add a resource or not is dependent on whether the user has the resource.add permission. In your method, you are checking whether the user is either an administrator or a data collector before setting the is_allowed_add_resource variable. This should be changed to check whether the user has the specific permission instead.
Please refer to the django-tutelary documentation to learn how to check whether the user has various permissions.
There was a problem hiding this comment.
Thanks for the feedback. I'll take a look and send the updated PR
| @@ -333,6 +333,7 @@ def get_context_data(self, **kwargs): | |||
| SpatialUnitGeoJsonSerializer( | |||
| self.object.spatial_units.all(), many=True).data | |||
| ) | |||
| context['allow_add_resource'] = self.is_allowed_add_resource | |||
There was a problem hiding this comment.
This view is inheriting the ProjectAdminCheckMixin which already sets the is_allowed_add_resource context variable. Then here, you are setting the allow_add_resource context variable, which is the same thing as is_allowed_add_resource. It would be better to just reuse the existing context variable.
There was a problem hiding this comment.
Got it. Thanks for the explanation. I'll correct it
|
Hi @manoramahp. I noticed that when you forked the repository and updated the code, you directly pushed your changes to your repository's |
Proposed changes in this pull request
This PR contains the fix for the issue #682
Please review and merge