-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add participants to hacknight endpoint with tests (fixes #93) #166
Changes from 8 commits
a33d8d6
c26beab
53e1724
1965377
7b38ef6
f773b4b
4cbb743
f9cfbd2
7954820
b983255
84d28ac
456c10c
cd311cf
fc80310
961a7d4
7030011
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,8 +8,9 @@ | |
from marshmallow import ValidationError | ||
|
||
from backend.extensions import db | ||
from backend.models import Hacknight | ||
from backend.models import Hacknight, Participant | ||
from backend.serializers.hacknight_serializer import HacknightSchema | ||
from backend.serializers.participant_serializer import ParticipantSchema | ||
|
||
|
||
class HacknightList(Resource): | ||
|
@@ -53,3 +54,36 @@ def get(self, id): | |
return {'hacknights': hacknight_schema.dump(Hacknight.query.get_or_404( | ||
id)) | ||
}, HTTPStatus.OK | ||
|
||
@jwt_required | ||
def patch(self, id): | ||
hacknight = Hacknight.query.get_or_404(id) | ||
participants = [ | ||
participant.id for participant in hacknight.participants | ||
] | ||
|
||
json_data = request.get_json(force=True) | ||
ids_schema = ParticipantSchema(only=('participants_ids',)) | ||
try: | ||
data = ids_schema.load(json_data) | ||
except ValidationError as err: | ||
return (err.messages), HTTPStatus.BAD_REQUEST | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no reason for |
||
|
||
new_participants = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Chenged to post on /hacknights//participants @ngiersz going to do delete method |
||
id for id in data['participants_ids'] if id not in participants | ||
] | ||
|
||
if not new_participants: | ||
return {'message': 'No new participant has been provided'}, \ | ||
HTTPStatus.BAD_REQUEST | ||
|
||
for new_participant in new_participants: | ||
hacknight.participants.append( | ||
Participant.query.get_or_404(new_participant) | ||
) | ||
db.session.add(hacknight) | ||
db.session.commit() | ||
|
||
hacknight_schema = HacknightSchema() | ||
return {"hacknight": hacknight_schema.dump(hacknight)}, \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No reason for |
||
HTTPStatus.OK |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,10 +4,12 @@ | |
class ParticipantSchema(Schema): | ||
class Meta: | ||
fields = ( | ||
'id', 'name', 'lastname', 'email', 'github', 'hacknights', 'phone' | ||
'id', 'participants_ids', 'name', 'lastname', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure, if that's a good wat to do that with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @w1stler please solve this discussion and merge There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could make it inline inside Resource:
and remove from participant serializer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. :) |
||
'email', 'github', 'hacknights', 'phone' | ||
) | ||
dump_only = ('id', 'hacknights') | ||
|
||
participants_ids = fields.List(fields.Int()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, how the |
||
name = fields.Str(required=True, validate=[validate.Length(max=50)]) | ||
lastname = fields.Str(validate=[validate.Length(max=50)]) | ||
email = fields.Email(validate=[validate.Length(max=200)]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't we agree to have separate url for that (like
/hacknights/<id>/participants
)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed