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

WIP: User Feedback for PickUps Tests #342

Merged
merged 55 commits into from Aug 31, 2017

Conversation

Projects
None yet
6 participants
@mddemarie
Copy link
Contributor

mddemarie commented Aug 10, 2017

We do a new WIP Pull request because we had to remove our GitHub branch feedback3 due to local merge conflict. But we saw the suggestions and we integrated them into our code.

Tiltec: we added 2 tests in stores/tests/test_feedback_api and they both work. There are 2 things we did not finish today: 1. pull last changes from master (we have a merge conflict there) 2. integrate read_only for given_by in serializers.py.

mddemarie and others added some commits Aug 2, 2017

deleted the signal post_feedback_create from signals.py and comment i…
…t out in serializers.py as well as some flake8 corrections
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 10, 2017

Codecov Report

Merging #342 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
+ Coverage   98.28%   98.34%   +0.06%     
==========================================
  Files         155      162       +7     
  Lines        4202     4358     +156     
  Branches      178      180       +2     
==========================================
+ Hits         4130     4286     +156     
  Misses         52       52              
  Partials       20       20
Impacted Files Coverage Δ
foodsaving/stores/models.py 96.49% <100%> (+0.16%) ⬆️
...aving/stores/migrations/0020_auto_20170831_1312.py 100% <100%> (ø)
foodsaving/stores/tests/test_feedback_api.py 100% <100%> (ø)
...aving/stores/migrations/0018_auto_20170831_1256.py 100% <100%> (ø)
...aving/stores/migrations/0019_auto_20170831_1306.py 100% <100%> (ø)
...aving/stores/migrations/0021_auto_20170831_1341.py 100% <100%> (ø)
foodsaving/stores/tests/test_models.py 100% <100%> (ø) ⬆️
foodsaving/stores/api.py 100% <100%> (ø) ⬆️
foodsaving/stores/migrations/0016_feedback.py 100% <100%> (ø)
foodsaving/stores/serializers.py 97.94% <100%> (+0.21%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4569c0b...de676d4. Read the comment docs.

@tiltec

This comment has been minimized.

Copy link
Member

tiltec commented Aug 10, 2017

I can't wait for the next test which is hopefully test_create_feedback_as_member and it shows how a feedback gets created 🍾 🎉 :)

(Sorry, I first wrote user instead of member)

@tiltec
Copy link
Member

tiltec left a comment

Hope my feedback helps you!

# serializer_class=FeedbackSerializer
# )

# returns 1 failure in tests

This comment has been minimized.

@tiltec

tiltec Aug 24, 2017

Member

You have two types of operations to consider when limiting access: retrievals (GET) and modifications (POST).

  1. For retrievals, you can filter with the queryset. I did this there for example.

This would be the code I'd try here:

queryset = FeedbackModel.objects  # make all feedback objects available by default

def get_queryset(self):
    # apply additional filters depending on the user who makes the request
    return self.queryset.filter(about__store__group__members=self.request.user)
  1. For modifications, you most likely need to validate the values given in the request body. This should be done in the serializer.

(This way, I think you can get rid of your detail(), get_permissions() and get_serializer_class() methods.)

This comment has been minimized.

@mddemarie

mddemarie Aug 24, 2017

Contributor

Thank you, this is definately helpful! 👍


"""
Tilmann suggested to make 'given_by' read_only - but the following code produces errors
extra_kwargs = {

This comment has been minimized.

@tiltec

tiltec Aug 24, 2017

Member

I would be interested to see what errors you get. Probably the database complains that the given_by field is missing a value, as it is non-nullable.

To solve this issue, you need to provide the given_by field when creating the entry. Override the create() method for that.

    def create(self, validated_data):
        validated_data['given_by'] = self.context['request'].user
        return super().create(validated_data)

I just noticed that you discovered the new read_only_fields shortcut. Great, please use that instead of extra_kwargs!

This comment has been minimized.

@mddemarie

mddemarie Aug 25, 2017

Contributor

Yes, we are getting errors also for read_only_fields. We will try it then with create() to avoid errors.

"""
self.client.force_login(user=self.user)
response = self.client.get(self.url)
self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND, response.data)

This comment has been minimized.

@tiltec

tiltec Aug 24, 2017

Member

I would expect an empty list instead of 404, similar to here

Explanation: the request goes to /api/feedback/, which is not group-specific. It will return the list of all feedback the user has access to, which is an empty list in this case.

This comment has been minimized.

@mddemarie

mddemarie Aug 25, 2017

Contributor

We have done that yesterday evening. You can look it up in our follow-up commit that I pushed.

@tiltec
Copy link
Member

tiltec left a comment

Looks very good! I added some minor suggestions.

@@ -53,6 +53,9 @@
router.register('invitations', InvitationsViewSet)
router.register('invitations', InvitationAcceptViewSet)

# Feedback endpoints
router.register(r'feedback', FeedbackViewSet, base_name='user_feedback')

This comment has been minimized.

@tiltec

tiltec Aug 31, 2017

Member

What is the base_name good for? Maybe it can be removed.

from foodsaving.utils.mixins import PartialUpdateModelMixin

pre_pickup_delete = Signal()
pre_series_delete = Signal()
post_store_delete = Signal()
post_feedback_delete = Signal()

This comment has been minimized.

@tiltec

tiltec Aug 31, 2017

Member

Seems not needed.

@@ -30,6 +30,13 @@ def __str__(self):
return '{} ({})'.format(self.name, self.group)


class Feedback(BaseModel):
given_by = models.ForeignKey('users.User', on_delete=models.CASCADE, related_name='user_feedback')

This comment has been minimized.

@tiltec

tiltec Aug 31, 2017

Member

Maybe change the related name to feedback, then it can be accessed from an user instance like this user.feedback instead of user.user_feedback.

pickup_date = about # PickupDateModel.objects.get(pk=about)
group = pickup_date.store.group
# is the member assiged to the pickup?
if user not in group.members.all():

This comment has been minimized.

@tiltec

tiltec Aug 31, 2017

Member

I will merge a pull request today #360 that allows you to use group.is_member(user) to reduce code duplication.

if user not in group.members.all():
raise serializers.ValidationError(_('You are not member of the store\'s group.'))
# if user is in self.context['request'].
if about not in self.context['request'].user.pickup_dates.all():

This comment has been minimized.

@tiltec

tiltec Aug 31, 2017

Member

Same as above, #360 also introduces about.is_collector(user). Looks a bit easier to understand then :)

This comment has been minimized.

@mddemarie

mddemarie Aug 31, 2017

Contributor

Ok, we know now what is needed to refactor. Thank you.

@@ -30,6 +30,13 @@ def __str__(self):
return '{} ({})'.format(self.name, self.group)


class Feedback(BaseModel):
given_by = models.ForeignKey('users.User', on_delete=models.CASCADE, related_name='user_feedback')
about = models.ForeignKey('PickupDate')

This comment has been minimized.

@nicksellen

nicksellen Aug 31, 2017

Member

I find the name about unintuitive, and it sounds like it's maybe a generic relation (but it's not) - how about pickup?

This comment has been minimized.

@tiltec

tiltec Aug 31, 2017

Member

I'm ok with both, change it if you agree with Nick and you feel like doing refactoring.

This comment has been minimized.

@mddemarie

mddemarie Aug 31, 2017

Contributor

we thought the same. about is too unintuitive. we will change it then.

This comment has been minimized.

@mddemarie

mddemarie Aug 31, 2017

Contributor

And are you sure that the name pickup will not crash with pickup at different places in code?


# if not self.context['request'].user.groups.filter(id=group_id).exists():
# raise serializers.ValidationError(_('You are not member of the store\'s group.'))
# return user_id

This comment has been minimized.

@nicksellen

nicksellen Aug 31, 2017

Member

Remove commented out code

# if self.action == 'retrieve':
# self.permission_classes = (IsAuthenticated, IsMember,)

# return super().get_permissions()

This comment has been minimized.

@nicksellen

nicksellen Aug 31, 2017

Member

Remove commented out code

@tiltec

tiltec approved these changes Aug 31, 2017

Copy link
Member

tiltec left a comment

🎇

@tiltec tiltec merged commit 149981f into master Aug 31, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
ci/circleci-e2e end-to-end test suite
Details
codecov/patch 100% of diff hit (target 98.28%)
Details
codecov/project 98.34% (+0.06%) compared to 4569c0b
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details

@tiltec tiltec deleted the feedback3 branch Aug 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment