-
Notifications
You must be signed in to change notification settings - Fork 0
Fixes #64 - Update current models to meet new design. #65
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
Fixes #64 - Update current models to meet new design. #65
Conversation
| __tablename__ = 'project' | ||
| id = db.Column(db.Integer, primary_key=True) | ||
| id = db.Column(db.String, primary_key=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.
Whenever you see a String that is related to a UUID, you should probably set a max number of characters, like 64 maybe.
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.
Also SQL Alchemy has multiple ways of generating UUID content and you need to do it for the attribute id.
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.
So that you are sure that such UUID works, you should probably change a bit the SQL Repository tests so that the model uses a UUID as primary key and verify that such id is actually being generated.
| description='Either identifier or locator', | ||
| example=faker.words( | ||
| 1, | ||
| ['http://example.com/mypage.html', '/some/page.html'] |
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 would specify something like a Jira ticket, e.g. TT-124 or a the URL of a Github issue, e.g. #51. Something like that, so this makes sense to the user.
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.
Nice work, there are a few comments
time_tracker_api/api.py
Outdated
| example='anonymous', | ||
| ), | ||
| } | ||
| """ |
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 not leave commented content
| "description": fake.paragraph(nb_sentences=2), | ||
| "start_date": fake.iso8601(end_datetime=None), | ||
| "end_date": fake.iso8601(end_datetime=None), | ||
| "owner_id": fake.uuid4(), |
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.
The model in LucidCharts has this field as owner only. @EliuX should we change the PR or update the LucidCharts diagram?
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.
Sure
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.
Just correct this tiny detail in all the missing fields and we should be ready to go
| name = db.Column(db.String(50), unique=True, nullable=False) | ||
| description = db.Column(db.String(250), unique=False, nullable=False) | ||
| deleted = db.Column(db.String, default=None) | ||
| tenant_id = db.Column(db.String, nullable=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.
Let's change all id fields that are supposed to be UUID to the right type. The use of String was actually a workaround.
No description provided.