-
Notifications
You must be signed in to change notification settings - Fork 37
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
populate subscription data for initial setup #70
Conversation
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.
See my line comments
yelp_beans/routes/tasks.py
Outdated
|
||
|
||
@tasks.route('/init', methods=['GET']) | ||
def init(): |
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.
Are we sure this should be its own route? I feel like this is better-suited to a one-off executable that we provide.
Similar to rake db:seed
that rails does
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.
Explored the option of using a script with the remote api. I was attempting to cut down on the user complexity of having to add remote_api: on
to the app.yaml, deploying, running a script, removing the line, and deploying again.
The route is only accessible by admins and is a noop if there is data, while not great from an engineering perspective, it seems to be the quickest upstart from a user perspective.
yelp_beans/routes/tasks.py
Outdated
@@ -92,3 +96,31 @@ def send_match_emails(): | |||
logging.info(matches) | |||
send_batch_meeting_confirmation_email(matches, spec) | |||
return "OK" | |||
|
|||
|
|||
@tasks.route('/init', methods=['GET']) |
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.
And this definitely shouldn't be a GET
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.
agreed
Looking for a better route to populate starter data. |
Helps initial state for people creating the app for the first time #23