-
Notifications
You must be signed in to change notification settings - Fork 0
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 content related story models and services #16
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.
Looks good to me? Added a couple questions
@@ -1,13 +1,16 @@ | |||
from flask_sqlalchemy import SQLAlchemy | |||
|
|||
db = SQLAlchemy() | |||
erase_db_and_sync = False | |||
erase_db_and_sync = True |
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.
Why do we want this to default to True
vs. toggling to clear as necessary?
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.
ah crap, my bad. good catch
translated_languages = db.Column(db.ARRAY(db.String)) | ||
contents = db.relationship(StoryContent) | ||
|
||
def to_dict(self, include_relationships=False): |
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.
Do you have an example of what this result looks like? Like even just to be added within this comment thread for reference
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.
very much what you might expect:
{
"id": 1,
"stage": START,
....
}
The funky to_dict is from starter-code. you could probably get away with just object.__dict__
for most things, but this fn will come in handy for including relationships too.
pass | ||
|
||
@abstractmethod | ||
def create_story(self, story): |
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 there use cases for the remaining CRUD operations (bulk or individual)?
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.
Yes - getting all stories will be a good spring board into the specific filters and conditions down the road. It's also good for mvp testing on the frontend side.
get_story by id will be needed for loading up the translation platform once a story is selected.
TFTR (thanks for the review)!! 🚢 |
Notion ticket link
Build stories and related models
Implementation description
New API
Tentative Queries
Column index decisions were made based on these queries. Each query uses the indexes to some degree.
Note about showing available stories
translated_languages
could've been a json dictionary for looking up if a language was translated or not. I tried testing the query in both conditions and there's a marginal improvement with the array. I predict the array will still perform better, but this is not too challenging to change in the future.Caution
stories.translatedLanguages
could technically fall out of sync from what is actually translated. We will need to keep it in sync withstory_translations
via apiChecklist
docker exec -it <backend-container-id> /bin/bash -c "black . && isort --profile black ."