WIP: User Feedback for PickUps Tests #342

mddemarie · 2017-08-10T15:48:47Z

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.

…t out in serializers.py as well as some flake8 corrections

…ous commit).

…into feedback3

into feedback3

… testing purpose

…ot a member of group

…ests

codecov-io · 2017-08-10T15:48:53Z

Codecov Report

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

@@            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 · 2017-08-10T17:03:17Z

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)

…edback3

…on-working code

… feedback3

…method GET (one fails at the moment)

…e user is non-member of group, see comments

…omments in apy.py

tiltec

Hope my feedback helps you!

tiltec · 2017-08-24T16:37:24Z

foodsaving/stores/api.py

+    #    serializer_class=FeedbackSerializer
+    # )
+
+    # returns 1 failure in tests


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

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)

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.)

Thank you, this is definately helpful! 👍

tiltec · 2017-08-24T17:41:38Z

foodsaving/stores/serializers.py

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


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!

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

tiltec · 2017-08-24T18:01:46Z

foodsaving/stores/tests/test_feedback_api.py

+        """
+        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)


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.

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

…, group member and collector

tiltec

Looks very good! I added some minor suggestions.

tiltec · 2017-08-31T11:44:52Z

config/urls.py

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

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


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

tiltec · 2017-08-31T11:45:21Z

foodsaving/stores/api.py

 from foodsaving.utils.mixins import PartialUpdateModelMixin

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


Seems not needed.

tiltec · 2017-08-31T11:47:57Z

foodsaving/stores/models.py

@@ -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')


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.

tiltec · 2017-08-31T11:49:30Z

foodsaving/stores/serializers.py

+        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():


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

tiltec · 2017-08-31T11:56:18Z

foodsaving/stores/serializers.py

+        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():


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

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

nicksellen · 2017-08-31T12:25:34Z

foodsaving/stores/models.py

@@ -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')


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

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

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

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

nicksellen · 2017-08-31T12:26:32Z

foodsaving/stores/serializers.py

+
+    #     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


Remove commented out code

nicksellen · 2017-08-31T12:26:43Z

foodsaving/stores/api.py

+    #    if self.action == 'retrieve':
+    #        self.permission_classes = (IsAuthenticated, IsMember,)
+
+    #    return super().get_permissions()


Remove commented out code

…ht and comment

tiltec

🎇

mddemarie and others added 20 commits August 2, 2017 12:35

added the correct API endpoint feedback

33da12d

added the correct API endpoint feedback

2e69dd5

corrected API endpoint feedback

b9f091a

Flaked some files

68ce735

changes with flake8

cb80da5

cleaned up stored.py

26cc533

minor changes in url routers

6baa704

Merge branch 'master' into feedback3

38b2a7e

playing with FeedbackSerializer and FeedbackModel

e150a0f

deleted the signal post_feedback_create from signals.py and comment i…

b249a20

…t out in serializers.py as well as some flake8 corrections

upload the changes in signals.py (which should habe been in the previ…

d104828

…ous commit).

Merge branch 'master' of https://github.com/yunity/foodsaving-backend …

c223d82

…into feedback3

Merge remote-tracking branch 'origin' into feedback3

232a3c9

fixing bugs

ee9e81e

Merge branch 'feedback3' of https://github.com/yunity/foodsaving-backend

2dc7ec7

into feedback3

commented out the fields given_by and about in the feedback model for…

946248b

… testing purpose

created a first test case, started with another test if the user is n…

abf0322

…ot a member of group

added a new test for which has to be written code in serializers.py

85f5aa5

added user validation for feedback serializer

6eaf4b0

removed unneeded code from the serializer and added comments to the t…

96cb616

…ests

id-gue and others added 8 commits August 11, 2017 10:26

master changes

f3ce6d7

added a new test and changed the Feedback validator

a39a2aa

bug fixing in test_feedback and playing with FeedbackSerializer

fdaec89

Merge branch 'master' of github.com:yunity/foodsaving-backend into fe…

33c96aa

…edback3

added 1 positive test for displaying feedback as user + uncommented n…

2e3d9f8

…on-working code

flake8

7046560

Merge branch 'feedback3' of github.com:yunity/foodsaving-backend into…

58d8639

… feedback3

Merge remote-tracking branch 'origin/master' into feedback3

911ce98

mddemarie and others added 8 commits August 22, 2017 13:39

Merge remote-tracking branch 'origin/master' into feedback3

06eae20

Merge remote-tracking branch 'origin/master' into feedback3

3d4372c

minor changes in FeedbackSerializer

9a90ddc

Merge remote-tracking branch 'origin/feedback3' into feedback3

7550e84

fixed the Feedback api test (members / users)

9177dbc

added permission IsMember to FeedbackViewSet and three tests for the …

6d66fe1

…method GET (one fails at the moment)

experimented with FeedbackViewSet, tried to raise Http404 error if th…

18ac4bf

…e user is non-member of group, see comments

turned one failing into passing test and added one more test + more c…

76f798b

…omments in apy.py

tiltec reviewed Aug 24, 2017

View reviewed changes

id-gue and others added 9 commits August 25, 2017 13:13

added tilmanns code to the serializer

6fcde9e

Merge remote-tracking branch 'origin/master' into feedback3

7e29335

added Feedback.objects.create() to SetUpClass in test_feedback_api.py

afea4d2

added queryset function to api to fix the FeedbackTests for list

97ca298

Merge remote-tracking branch 'origin/master' into feedback3

a852bab

Merge remote-tracking branch 'origin/feedback3' into feedback3

57e97e0

more tests for getting single feedback with GET.id for non-user, user…

65764cd

…, group member and collector

1 more test for the feedback in the future + comment clean-up

a622ed1

Merge remote-tracking branch 'origin/master' into feedback3

fe5c149

tiltec reviewed Aug 31, 2017

View reviewed changes

nicksellen reviewed Aug 31, 2017

View reviewed changes

id-gue and others added 5 commits August 31, 2017 15:17

test the feedback model and changed the parameters of the fields weig…

e99ba1a

…ht and comment

Merge remote-tracking branch 'origin/master' into feedback3

7889a69

Merge remote-tracking branch 'origin/feedback3' into feedback3

f4d790a

removed comments and all unnecessary code

d58cb45

test if feedback can be created if comment to long

de676d4

tiltec approved these changes Aug 31, 2017

View reviewed changes

tiltec merged commit 149981f into master Aug 31, 2017

tiltec deleted the feedback3 branch August 31, 2017 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: User Feedback for PickUps Tests #342

WIP: User Feedback for PickUps Tests #342

mddemarie commented Aug 10, 2017

codecov-io commented Aug 10, 2017 •

edited by codecov bot

tiltec commented Aug 10, 2017 •

edited

tiltec left a comment

tiltec Aug 24, 2017 •

edited

mddemarie Aug 24, 2017

tiltec Aug 24, 2017 •

edited

mddemarie Aug 25, 2017

tiltec Aug 24, 2017

mddemarie Aug 25, 2017

tiltec left a comment

tiltec Aug 31, 2017

tiltec Aug 31, 2017

tiltec Aug 31, 2017

tiltec Aug 31, 2017

tiltec Aug 31, 2017

mddemarie Aug 31, 2017

nicksellen Aug 31, 2017

tiltec Aug 31, 2017

mddemarie Aug 31, 2017

mddemarie Aug 31, 2017

nicksellen Aug 31, 2017

nicksellen Aug 31, 2017

tiltec left a comment

WIP: User Feedback for PickUps Tests #342

WIP: User Feedback for PickUps Tests #342

Conversation

mddemarie commented Aug 10, 2017

codecov-io commented Aug 10, 2017 • edited by codecov bot

Codecov Report

tiltec commented Aug 10, 2017 • edited

tiltec left a comment

Choose a reason for hiding this comment

tiltec Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiltec Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiltec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiltec left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 10, 2017 •

edited by codecov bot

tiltec commented Aug 10, 2017 •

edited

tiltec Aug 24, 2017 •

edited

tiltec Aug 24, 2017 •

edited