Add participants to hacknight endpoint with tests (fixes #93) #166

stanislawK · 2019-08-31T12:47:00Z

Testing:

make populate_database
make start
login in postman with given username: "/auth/login"
copy access_token
send patch request to "hacknights/1/":
Headers: Authorization: Bearer access_token; Content-Type: application/json
Body: {
"participants": [1]
}

With pytest:

make bash
type pytest and press enter

w1stler

Overal seems okay, just have some questions - to be asked on next meeting.

w1stler · 2019-09-11T19:22:36Z

backend/resources/hacknight.py

+            return {'message': 'No new participant has been provided'}, \
+                HTTPStatus.BAD_REQUEST
+
+        for new_particpiant in new_participants:


new_participant - just typo

w1stler · 2019-09-11T19:23:01Z

backend/resources/hacknight.py

+
+        for new_particpiant in new_participants:
+            hacknight.participants.append(
+                Participant.query.get_or_404(new_particpiant)


w1stler · 2019-09-11T19:34:15Z

backend/tests/test_hacknight.py

+def test_add_participants_to_nonexistent_hacknight(
+    access_token, add_participants, client
+):
+    """Test add participants to non-existent hacknight."""


non-existing

w1stler · 2019-09-11T19:35:20Z

backend/tests/test_hacknight.py

+def test_duplicate_participant_in_hacknight(
+    access_token, add_hacknights, add_participants, client, _db
+):
+    """Test add participant who is in hacknight already."""


Add participant who is already in hacknight.

It's not perfect, but sounds better. ;-)

w1stler · 2019-09-11T19:35:32Z

backend/tests/test_hacknight.py

+    assert response['msg'] == 'Missing Authorization Header'
+
+
+def test_add_nonexistent_participants_to_hacknight(


non-existing

w1stler · 2019-09-11T19:40:42Z

backend/serializers/participant_serializer.py

@@ -18,3 +18,10 @@ class Meta:
        many=True
    )
    phone = fields.Str(validate=[validate.Length(max=13)])
+
+
+class ParticipantIdSchema(Schema):


I'd like to discuss this approach.

w1stler · 2019-10-23T17:44:25Z

backend/tests/test_hacknight.py

+def test_duplicate_participant_in_hacknight(
+    access_token, add_hacknights, add_participants, client, _db
+):
+    """Test add participant who is already in hacknight.."""


Redundant space at the end.

w1stler · 2019-10-23T17:47:34Z

backend/tests/test_hacknight.py

+    assert rv.status_code == HTTPStatus.OK
+
+    hacknight_db = Hacknight.query.get(1)
+    ids_from_db = [part.id for part in hacknight_db.participants]


Why using part instead of participant here? I think it won't collide with further code (you're using ids_from_db).

w1stler · 2019-10-23T18:17:28Z

backend/tests/test_hacknight.py

+        data=json.dumps(payload)
+    )
+    resp = rv.get_json()
+    participants_ids = [


That's never used

w1stler · 2019-10-23T18:18:52Z

backend/tests/test_hacknight.py

+        headers={'Authorization': 'Bearer {}'.format(access_token)},
+        data=json.dumps(payload)
+    )
+    response = rv.get_json()


That's never used.

w1stler · 2019-10-23T18:19:06Z

backend/tests/test_hacknight.py

+        headers={'Authorization': 'Bearer {}'.format(access_token)},
+        data=json.dumps(payload)
+    )
+    response = rv.get_json()


That's never used.

w1stler · 2019-10-23T19:00:21Z

backend/serializers/participant_serializer.py

@@ -4,10 +4,12 @@
 class ParticipantSchema(Schema):
    class Meta:
        fields = (
-            'id', 'name', 'lastname', 'email', 'github', 'hacknights', 'phone'
+            'id', 'participants_ids', 'name', 'lastname',


I am not sure, if that's a good wat to do that with participant_ids.

At first I delegated it to separate schema, but as we discussed, it wasn't best solution. So I pull it into ParticipantSchema. Do you have some suggestions. I certenly want serialize somehow that payload.

@w1stler please solve this discussion and merge

I could make it inline inside Resource:

ids_schema = Schema.from_dict({"participants_ids": fields.List(fields.Int())}) try: data = ids_schema().load(json_data)

and remove from participant serializer.
What do you think about that solution?

Seems that is a better solution. We can find some examples with good practices for similar problem, as I am still not 100% confident if that's the best approach. :)

magul

There's one merge conflict to be resolved, but I also sincerily believe there's a better place to put these participants ids.

magul · 2019-11-09T14:37:30Z

backend/serializers/participant_serializer.py

        )
        dump_only = ('id', 'hacknights')

+    participants_ids = fields.List(fields.Int())


Wait, how the participants_ids as a list can be part of a ParticipantSchema? I can agree, that list in such shape will be used to attach participants to a hacknight, but it should be defined in probably different place.

magul · 2019-11-09T14:48:18Z

Ok, I see that you're working on that @stanislawK. Just, please, notify me when it will be ready. Also if you will feel that you're struggling with that we can have a conversation about that next hacknight.

w1stler · 2019-11-20T21:44:18Z

backend/resources/hacknight.py

+        db.session.commit()
+
+        hacknight_schema = HacknightSchema()
+        return {"hacknight": hacknight_schema.dump(hacknight)}, \


I think we can remove "hacknight".

No reason for hacknight as a key here, we should return it directly.

w1stler · 2019-11-27T17:51:10Z

Do we have an agreement here @stanislawK & @magul ?

magul

A few questions to be addressed here.

magul · 2019-11-29T00:09:31Z

backend/resources/hacknight.py

+        try:
+            data = ids_schema().load(json_data)
+        except ValidationError as err:
+            return (err.messages), HTTPStatus.BAD_REQUEST


no reason for () here, also 422 could be applied here.

magul · 2019-11-29T00:10:18Z

backend/resources/hacknight.py

+        db.session.commit()
+
+        hacknight_schema = HacknightSchema()
+        return {"hacknight": hacknight_schema.dump(hacknight)}, \


No reason for hacknight as a key here, we should return it directly.

magul · 2019-11-29T00:11:24Z

backend/resources/hacknight.py

@@ -54,3 +54,38 @@ def get(self, id):
        return {'hacknights': hacknight_schema.dump(Hacknight.query.get_or_404(
            id))
        }, HTTPStatus.OK
+
+    @jwt_required
+    def patch(self, id):


didn't we agree to have separate url for that (like /hacknights/<id>/participants)?

I hear it for first time. No one discussed it with me, although there was plenty of time - it is 4 months since I proposed this pr.

magul · 2019-11-29T00:15:03Z

backend/resources/hacknight.py

+        except ValidationError as err:
+            return (err.messages), HTTPStatus.BAD_REQUEST
+
+        new_participants = [


As I understand that this endpoint should be able to change the list of participants for particular hacknight.

I see that you implemented only adding new participants? How the plan for removing a participant from hacknight looks like?

Title of these ticket is "Create add participant to hacknight endpoint", and I believe that delete participant from hacknight, is great topic for next one

Chenged to post on /hacknights//participants @ngiersz going to do delete method

dismissed

Add participants to hacknight endpoint with tests

a33d8d6

w1stler requested a review from a team September 4, 2019 18:09

w1stler reviewed Sep 11, 2019

View reviewed changes

stanislawK added 6 commits September 18, 2019 19:02

Fixed typos in hacknight's resource and tests

c26beab

Merge branch 'master' into 93_add_participant_to_hacknight_endpoint

53e1724

fixed js lint errors

1965377

Added participants_ids field to ParticipanSchema

7b38ef6

Conflicts resolved

f773b4b

Deleted ParticipanIDSchema

4cbb743

w1stler reviewed Oct 23, 2019

View reviewed changes

stanislawK added 3 commits November 6, 2019 20:06

Fixes in test_hacknight

f9cfbd2

Merge branch 'master' into 93_add_participant_to_hacknight_endpoint

7954820

Removed participants_ids field in ParticipantSchema

b983255

magul requested changes Nov 9, 2019

View reviewed changes

stanislawK added 3 commits November 9, 2019 15:50

Conflict resolved

84d28ac

Merge branch 'master' into 93_add_participant_to_hacknight_endpoint

456c10c

Pulled changes in participant serializer from master

cd311cf

w1stler reviewed Nov 20, 2019

View reviewed changes

magul previously requested changes Nov 29, 2019

View reviewed changes

stanislawK added 3 commits December 7, 2019 15:24

Changed return message

fc80310

Merge branch 'master' into 93_add_participant_to_hacknight_endpoint

961a7d4

Changed resource path

7030011

w1stler approved these changes Dec 20, 2019

View reviewed changes

w1stler merged commit bfcf68f into CodeForPoznan:master Jan 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add participants to hacknight endpoint with tests (fixes #93) #166

Add participants to hacknight endpoint with tests (fixes #93) #166

stanislawK commented Aug 31, 2019

w1stler left a comment

w1stler Sep 11, 2019

w1stler Sep 11, 2019

w1stler Sep 11, 2019

w1stler Sep 11, 2019

w1stler Sep 11, 2019

w1stler Sep 11, 2019

w1stler Oct 23, 2019

w1stler Oct 23, 2019

w1stler Oct 23, 2019

w1stler Oct 23, 2019

w1stler Oct 23, 2019

w1stler Oct 23, 2019

stanislawK Nov 6, 2019

OtisRed Nov 6, 2019

stanislawK Nov 6, 2019

w1stler Nov 9, 2019

magul left a comment

magul Nov 9, 2019

magul commented Nov 9, 2019

w1stler Nov 20, 2019

magul Nov 29, 2019

w1stler commented Nov 27, 2019

magul left a comment

magul Nov 29, 2019

magul Nov 29, 2019

magul Nov 29, 2019

stanislawK Dec 7, 2019

stanislawK Dec 11, 2019

magul Nov 29, 2019

stanislawK Dec 7, 2019

stanislawK Dec 11, 2019

		assert response['msg'] == 'Missing Authorization Header'


		def test_add_nonexistent_participants_to_hacknight(

Add participants to hacknight endpoint with tests (fixes #93) #166

Add participants to hacknight endpoint with tests (fixes #93) #166

Conversation

stanislawK commented Aug 31, 2019

w1stler 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

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

magul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magul commented Nov 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w1stler commented Nov 27, 2019

magul 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