[refactor] Use FilterByParent mixin in BaseEmailView #354

[dee077]

Refactored BaseEmailView to inherit from FilterByParent mixin.

Fixes #354

Checklist

• I have read the OpenWISP Contributing Guidelines.
• I have manually tested the changes proposed in this pull request.

Reference to Existing Issue

Closes #354 .

Description of Changes

I have removed the assert_parent_exists method and in BaseEmailView as It is present in FilterByParent and in the 'assert_parent_exists' method in FilterByParent made some changes which I am not sure to be correct but all the test cases are passing now.

I have a few questions as well

1. In between FilterByParent, FilterByParentMembership, FilterByParentManaged, and FilterByParentOwned which class to inherit I have gone for FilterByParentOwned.

2. Is this okay to remove this in the method assert_parent_exists in class FilterByParent? All test cases are passing after the removal as well.

      if not self.request.user.is_superuser:
      parent_queryset = self.get_organization_queryset(parent_queryset)

Removed the above code because it is causing issues, in the get_organization_queryset method the self.organization_lookup from the OrgLookup class has organization__in inside it but the field in the EmailAddress model is openwisp_user_orginization not organization causing FieldError that's why decided to remove this.

3. Should add a logic in the OrgLookup class to add organization_lookup correctly or removing the code is fine?

Please give your thoughts on this!

[coveralls]

coverage: 97.775% (-0.01%) from 97.787%
when pulling 2499b65 on dee077:issues/354-refactor-baseemailview
into 37fd040 on openwisp:master.

[nemesifier]

This doesn't look like something that was requested, why are you changing this?

[dee077]

In the get_organization_queryset method,

def get_organization_queryset(self, qs):
        lookup = {self.organization_lookup: getattr(self.request.user, self._user_attr)}
        return qs.filter(**lookup)

self.organization_lookup has the value organization__in, but in the User model, there is no direct link to Organization. It is managed by OrganizationUser to obtain many-to-many relationships between the Organization and User models. So, it adds openwisp_user as the prefix of the organization field and makes the field name openwisp_user_organization. However, we are searching for the field name organization, which cannot be found and causes a field error.

What I have gone through:

1. Remove this if condition if not self.request.user.is_superuser as it was not used in the previous implementation of BaseEmailView assert_parent_exists method. All test case passes so I have gone for this.
2. I have tried changing the lookup to check for the openwisp_user_organization field instead, but it causes other test failures. The models defined in /test folder like Config, Book, Shelf etc. have ForeignKey to the organization so test on these expecting field names as organization.

Please provide your thoughts on this

[nemesifier]

I have a few questions as well

1. In between FilterByParent, FilterByParentMembership, FilterByParentManaged, and FilterByParentOwned which class to inherit I have gone for FilterByParentOwned.

Why? What's the reasoning behind this choice?

2. Is this okay to remove this in the method assert_parent_exists in class FilterByParent? All test cases are passing after the removal as well.

      if not self.request.user.is_superuser:
      parent_queryset = self.get_organization_queryset(parent_queryset)

Removed the above code because it is causing issues, in the get_organization_queryset method the self.organization_lookup from the OrgLookup class has organization__in inside it but the field in the EmailAddress model is openwisp_user_orginization not organization causing FieldError that's why decided to remove this.

The fact that no test fails after the removal is concerning, it shows a lack of test coverage, but I am pretty sure it is not ok to remove this code.

3. Should add a logic in the OrgLookup class to add organization_lookup correctly or removing the code is fine?

Can you explain why you are compelled to change the internal logic of these classes instead of focusing on what the issue description is asking?

I don't understand your reasoning behind these changes. Please explain so we can understand.

[nemesifier]

I think this should be:

Suggested change

	class BaseEmailView(ProtectedAPIMixin, FilterByParentOwned, GenericAPIView):
	class BaseEmailView(ProtectedAPIMixin, FilterByParentManaged, GenericAPIView):

That allows organization managers to use the endpoint.