#32: Add ReservationSettings model #53
Conversation
|
Job #53 is now in scope, role is |
|
This pull request #53 is assigned to @amihaiemil/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @emilianodellacasa/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be a monetary reward for this job |
There was a problem hiding this comment.
@vladarefiev some comments :)
and please, specify the original ticket number in the PR's description. Start the description with "PR for ...:" :D
| "6": "Good morning", | ||
| "12": "Good afternoon", | ||
| "18": "Good morning", | ||
| } |
There was a problem hiding this comment.
@vladarefiev Indentation here
There was a problem hiding this comment.
@amihaiemil excuse me, didn't get your point.
| greeting_by_time = DB.Column(DB.JSON) | ||
| sex = DB.Column(DB.String) | ||
| created_on = DB.Column( | ||
| DB.DateTime, default=datetime.utcnow, nullable=False) |
There was a problem hiding this comment.
@vladarefiev Why is this on a new line, can't everything be on one line? Is some linter complaining?
There was a problem hiding this comment.
@amihaiemil Actually according to PEP8
Limit all lines to a maximum of 79 characters
timeless/reservations/views.py
Outdated
| bp = Blueprint('reservations', __name__, url_prefix='/reservations') | ||
|
|
||
|
|
||
| @bp.route('/settings_page') |
There was a problem hiding this comment.
@vladarefiev just /settings here, I would say. This is the API layer, it doesn't know anything about "pages", it just returns and consumes Json objects, right? :D
timeless/reservations/views.py
Outdated
| """ | ||
| @todo #32:30min Continue implementing Settings page | ||
| """ | ||
| return 'Settings page' |
There was a problem hiding this comment.
@vladarefiev "Settings API entry point" is more suitable.
| from flask import Blueprint | ||
|
|
||
|
|
||
| bp = Blueprint('reservations', __name__, url_prefix='/reservations') |
There was a problem hiding this comment.
@vladarefiev Shouldn't /reservations be tied to locations? Some reservations are made in one particular location, so I would expect the path to be something like {domain}/locations/{location_id}/reservations/...
There was a problem hiding this comment.
@amihaiemil As far as I can see this should be {domain}/reservations/settings which will contain default settings for resevation creation.
WDYT?
There was a problem hiding this comment.
@vladarefiev well, yes, reservations/settings will be the settings for reservations, but /reservations should be under a location (e.g. /locations/{locationsId}/reservations/.... Let's ask @emilianodellacasa -- if it's necessary, you can leave a puzzle, I believe :)
There was a problem hiding this comment.
@amihaiemil Ah, I think, I got your point. Do you mean every location has own settings?
@emilianodellacasa Could you clarify this one?
Anyway I should leave puzzle here to implement nested bluprint routes
There was a problem hiding this comment.
@vladarefiev @amihaiemil '/reservations' endpoint will be deployed on a different subdomain, as stated in issue #27, no need to tie it to the location
|
@amihaiemil pls take a look again. |
|
@vladarefiev there are some conflicts now, pls fix them :) |
|
@emilianodellacasa this is good to merge, from my point of view, but see our discussion first -- wdyt? |
|
@amihaiemil Take a look pls, conflicts are fixed |
|
@rultor good to merge |
@amihaiemil Thanks for your request. @emilianodellacasa Please confirm this. |
|
@rultor merge |
@emilianodellacasa OK, I'll try to merge now. You can check the progress of the merge here |
@emilianodellacasa @vladarefiev Oops, I failed. You can see the full log here (spent 3min) |
|
@vladarefiev Puzzle description is too short, please modify it and I will merge |
|
@emilianodellacasa done |
|
@rultor merge |
@emilianodellacasa OK, I'll try to merge now. You can check the progress of the merge here |
@emilianodellacasa @vladarefiev Oops, I failed. You can see the full log here (spent 3min) |
|
@vladarefiev it seems the puzzle is still too short. @emilianodellacasa maybe we can take out this check from rultor? I've noticed in other project's of mine that |
|
@amihaiemil I don't think that check can be removed, and I don't want to take pdd out of rultor, otherwise puzzles may not be created @vladarefiev Please add a more meaningful description for the puzzle |
|
@emilianodellacasa Done, now it's more then 20 |
|
@rultor merge |
@emilianodellacasa OK, I'll try to merge now. You can check the progress of the merge here |
@emilianodellacasa @vladarefiev Oops, I failed. You can see the full log here (spent 3min) |
|
@rultor merge |
@emilianodellacasa OK, I'll try to merge now. You can check the progress of the merge here |
@emilianodellacasa Done! FYI, the full log is here (took me 3min) |
|
Job was finished in 19 hours, bonus for fast delivery is possible (see §36) |
|
@ypshenychka/z please review this job completed by @amihaiemil/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #53 is now out of scope |
|
Payment to |
|
@0crat quality good |
|
Order was finished, quality is "good": +25 point(s) just awarded to @amihaiemil/z |
|
Quality review completed: +8 point(s) just awarded to @ypshenychka/z |
PR for #32
views.pyfor reservation module