Skip to content
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

feat: add new tables and relations for user schema #26

Merged
merged 9 commits into from
Sep 21, 2024

Conversation

aarbanas
Copy link
Owner

@aarbanas aarbanas commented Jul 29, 2024

Description

With this PR we've extended the existing user schema with new requirements from the client.

How to test

  1. Drop local database
drop database "red-cross-evidence"
  1. Create new empty database
create database "red-cross-evidence"
  1. From the root run next commands in the same order:
$ yarn db:migrate
$ yarn db:seed

Change summary

  • model update in user schema
  • introduce new migration files

Checklist

  • I have performed a self-review of my code
  • I tested locally
  • I have commented on my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New package or package update introduced

@aarbanas aarbanas self-assigned this Jul 29, 2024
@aarbanas aarbanas linked an issue Jul 29, 2024 that may be closed by this pull request
@aarbanas aarbanas marked this pull request as ready for review August 8, 2024 20:12
@aarbanas aarbanas requested a review from IMilja as a code owner August 8, 2024 20:12
@IMilja
Copy link
Collaborator

IMilja commented Aug 31, 2024

@aarbanas I finally had some time to look at the DB design and model. Overall I think we have a good model regarding everything. There are some additional stuff that I need to understand primarily it's the work_status and language model. I'm gonna write my questions/concerns below since I'm not 100% sure what the correct requirements are.

  1. We're storing the language_id column in a profile table which will allow the users only to store/input one single language. I would argue that it's better to allow users to select multiple languages that they know.
    image

  2. We're storing the size_id column in a profile table which will require you to link profile_size in what I think is the reverse "order" of how the relation should be. One-to-one relations are usually pretty tricky on the topic of who's gonna be the owning side of the relation and for this case, I think it should be the profile_size table with a profile_id column. The primary reason for this is that you can imagine if you for example want to delete a user account for GDPR purposes you can set CASCADE rules on the profile_size table and it's gonna be automatically deleted due to the profile_id column.

  3. Similar to my previous points, the table work_status is also currently being implemented so that only one works status can exist at a given time. Again, since I do not know the exact business requirements this might be the correct design, but if you're gonna ever have a requirement to show a "history of employment" it's gonna be impossible to be done using this DB model.

If you have any questions we can talk about this on Discord so feel free to ping me 🚀! Everything else looks perfect, and my DB OCD likes it :P singular tables names with snake case naming convention :D

@aarbanas
Copy link
Owner Author

aarbanas commented Sep 1, 2024

@IMilja ty for the review 🚀

  1. For the languages, you are correct and I will fix it
  2. We can discuss this one over Discord
  3. There would be no "history" is it is just one work_status

@IMilja
Copy link
Collaborator

IMilja commented Sep 21, 2024

@aarbanas I've checked the PR a bit and I found one smaller issue with the address table. The is_primary should be on the many-to-many relation since it's specific to a user with a relation to an address. Currently, the is_primary would indicate that one address is primary for everyone who picks it.

image

@IMilja
Copy link
Collaborator

IMilja commented Sep 21, 2024

@aarbanas Don't hate me 😢 I'm trying to not nitpick but we have another scenario in the profile_languages part. Primarily, I've noticed that we have multiple languages based on a level which in the end looks like this:

image

This is not wrong, but, not ideal either because you have now 6 records per language in the DB. What I would suggest that we can look into is the following.

We drop the level column from the language table and move it to profile_language. In that way, you're gonna remove the need for data duplication and also disallow a user to pick the same language but with a different level on a DB/Schema level.

image

One other benefit I can see later is an easier implementation of a feature where you can change the level (if you completed a course) because when you need to update a level you just need a profile_id and language_id to target a specific record, but, with the current approach you would need to find the current selection, remove it from profile_language and insert a new record.

@IMilja
Copy link
Collaborator

IMilja commented Sep 21, 2024

@aarbanas everything else has my blessing 🚀. Nice first from 0 DB design!!!

@aarbanas aarbanas merged commit cf17a55 into main Sep 21, 2024
1 check passed
@aarbanas aarbanas deleted the feat/extend-user-schema branch September 21, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend user schema with new requirements from the client
2 participants