-
Notifications
You must be signed in to change notification settings - Fork 2
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
Upload API Prep Part 2: Refactor database and api to new table format #35
Conversation
francisduvivier
commented
Nov 27, 2024
- Includes adding and using the sql-template-tag package, for increased safety and readability of sql code.
- Includes putting domain layer between api and db, ddd style
bbd055c
to
da8d725
Compare
beae63c
to
a94a1e4
Compare
a94a1e4
to
2a470fd
Compare
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.
With some understanding of DBs I did review the DB migration parts which seemed okay and I have not much knowledge of JS to comment on the implementations. As far as I am concerned, this can be merged.
( | ||
id text primary key, -- using text as recommended | ||
email text unique, -- using text as recommended | ||
admin boolean, |
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.
Still feel like we should use roles for this instead, i.e. a separate table user_roles
with an FK to users.id
and a 1 -> n relationship, so that users can have multiple roles
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.
Yeah haven't thought about this at all yet, the users table is actually just taken over from what we already had before this PR. (Except that I changed the user id to a string here in this PR, but then I changed it back to what we had in the next 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.
Let;s talk about it on Sunday to tie all this together with the middleware too
password text not null, | ||
remember_token text, | ||
editor text, | ||
public boolean, |
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.
What is the difference between public
and show_projects
?
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.
No Idea honestly.
Did not look into User data at all yet except for being able to relate them to a project.
Maybe @edwinm knows?
email text unique, -- using text as recommended | ||
admin boolean, | ||
name text not null, | ||
password text not null, |
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 salting these?
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 no idea. But actually good point. Gonna remove the user api for now so that this code for user insertion does not get used in production eventually accidentally.
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.
slug text not null primary key, | ||
git text, | ||
allow_team_fixes boolean, | ||
constraint projects_user_id_fk foreign key (user_id) references users (id) on delete cascade -- if user is deleted, delete his projects |
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.
delete their projects
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 true, if a user really get's deleted out of the DB as opposed to the deleted_at being set, then all the users' projects get deleted as well.
This way we can keep a more clean DB with stronger constraints about the validity of the data.
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.
No I meant their
as in, singular they (which is gender neutral in English)
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.
Oh sorry completely missed that, let me update that.
Edit: added a commit to fix it: it 7f679a1
c1330de
to
a6af53e
Compare
2a470fd
to
c290e27
Compare
…ot duplicating data We also make a layer between the API and the database in the style of DDD In order to write more readable queries, we also introduce the sql-template-tag library here
7f679a1
to
15fdef0
Compare