rederlyhq / backend

This is the core backend for Rederly. Essentially this is the glue and business logic that connects everything together. For the most part, the frontend makes requests to the backend which then fetches data from the backend or makes appropriate requests to microservices.
GNU General Public License v3.0
1 stars 2 forks source link

Database audit #31

Closed tommy-lettieri closed 3 years ago

tommy-lettieri commented 4 years ago

Make sure the database aligns with @Jhuanderson-Rederly 's db diagram, if not determine what needs to change and leave a comment with required changes for db diagram

tommy-lettieri commented 4 years ago
tommy-lettieri commented 4 years ago
tommy-lettieri commented 4 years ago

Add overall_best_score from #47 to grades

tommy-lettieri commented 4 years ago

@Jhuanderson-Rederly I looked over the db diagram and have these notes so far::

university

discrepancies

constraints

university_curriculum_permission

discrepancies

constraints

permission

discrepancies

Needs more definition

constraints

user_role

discrepancies

constraints

users

discrepancies

constraints

curriculum

discrepancies

constraints

curriculum_unit_content

discrepancies

constraints

curriculum_topic_content

discrepancies

constraints

curriculum_topic_question

discrepancies

constraints

course

discrepancies

constraints

course_unit_content

discrepancies

constraints

course_topic_content

discrepancies

constraints

course_topic_question

discrepancies

student_enrollment

discrepancies

constraints

student_workbook

discrepancies

constraints

student_grade

discrepancies

constraints

tommy-lettieri commented 4 years ago

Schema Comparison Setup:

Java 8

I already had java 8

Liquibase

Notes

Diff example

Command

The Test

tommy-lettieri commented 4 years ago

@Jhuanderson-Rederly The following errors prevent the export of dbdiagram.io from being used to generate a database (used for diffing purposes)

user

Foreign keys

The following foreign keys resulted in error so I commented them out for now:

-- ALTER TABLE "course_topic_content" ADD FOREIGN KEY ("curriculum_topic_content_id") REFERENCES "curriculum_topic_question" ("curriculum_topic_content_id");
-- ALTER TABLE "topic_type" ADD FOREIGN KEY ("topic_type_id") REFERENCES "course_topic_content" ("topic_type_id");
-- ALTER TABLE "university" ADD FOREIGN KEY ("university_id") REFERENCES "users" ("university_id");
-- ALTER TABLE "permission" ADD FOREIGN KEY ("permission_name") REFERENCES "user_role" ("user_role_permission");
tommy-lettieri commented 4 years ago

Only some of the tables have last_edited and created_at timestamps, should they all have that?

tommy-lettieri commented 4 years ago

"users" is plural while the other tables are all singular, I assume this was done to avoid conflicts with internal database tables, can we rename it to something like "system_user" to keep plurality consistent? If not can we change the prepended table name on the fields to users instead of user?

tommy-lettieri commented 4 years ago

I did the audit based on this db diagram: https://dbdiagram.io/d/5ebe13d439d18f5553ff3be6, There was one more that came out but it seems to have features (user group) that were not complete or decided on

Jhuanderson-Rederly commented 4 years ago

@Jhuanderson-Rederly I looked over the db diagram and have these notes so far::

university

discrepancies

  • prof_email_domain should be university_prof_email_domain
  • student_email_domain should be university_student_email_domain

agreed.

constraints

  • each field should have unique constraint

university_curriculum_permission

discrepancies

constraints

  • combo unique: [university_id, curriculum_id]

Yes, university would only need permission once for each unqiue currciulum. Though what proccess would cause a univerity to have permission to same curriculum within the databse? The constrant is okay!

permission

discrepancies

Needs more definition

For right now we only have Professor and Student. "can do" "can that" can be deleted from the database. The logic to handle their actual view in the page is handled by the front end.

constraints

  • unique: name

That is a proper constraint

user_role

discrepancies

constraints

  • user_id should be unique? can a user have mutliple roles (assuming no)?

A user can have mulitple permissions. Don't see why professor cant be admin or researchers ( future feature)

users

discrepancies

  • changed user_username to user_first_name and user_last_name
  • added user_verify_token: string
  • added user_verified: boolean

okay!

constraints

  • unique: user_verify_token
  • unique: user_email

okay!

curriculum

discrepancies

  • subject should be curriculum_subject

Initially I thought we could have a subject table. So that we limit the users can put in the field, also reduce mispelling. Last thing we want is calculus & Calc and having us to figure out that they are the same thing. Thoughts?

constraints

  • unique: name

Yes please!

curriculum_unit_content

discrepancies

  • add curriculum_unit_content_order

okay!

constraints

  • unique: name
  • unique: [curriculum_unit_content_order, curriculum_id]

Are we talking about unique per curriclulum? if so, I am on onaboard.
Context: Units are just arbitrary. I don't think we should add the name constants. Professors can call their units whatever they want. Even is it doesn't make sense. From an analytics perspective, we care more about the topic level than units.

curriculum_topic_content

discrepancies

  • add curriculum_topic_content_order
  • add topic type

Okay!

constraints

  • unique: name
  • unique: [curriculum_topic_content_order, curriculum_unit_content_id]

Are we talking about unique per curriclulum per unit? if so, I am on onaboard.

curriculum_topic_question

discrepancies

  • webwork_question_ww_path should be curriculum_topic_question_webwork_question_ww_path

I didnt want to add the pretext becuase the path does not belong to the table. That data is being stored elsewhere. My general rule is if the data belongs to the table add the pre-text, else if column used for join or data exists outside of the table then don't. I will do w.e is easiest. It doesn't matter much.

constraints

  • unique: [problem number, topic id]

Each problem # should only appear once per topic. Cant have 5s floating around. Okay! - We can prob extend to ad: unique: [problem number, topic id, path] * lets ask to Drew.

course

discrepancies

constraints

  • unique: name
  • unique: code
  • unique: section code
  • unique: semester code
  • end date after start date

Okay!

course_unit_content

discrepancies

  • course_content_unit_name should be course_unit_content_name
  • add course_content_order
  • add curriculum_unit_content_id?

Okay! && add curriculum_unit_content_id? For consistency we can if its an easy add. I purposely left it out because I saw little value at the time.

constraints

  • name unique?
  • unique: [course_content_order, course_id]

Same as in curriculum. unique per course_id Okay!

course_topic_content

discrepancies

  • start, due, dead should have date after them
  • add course_topic_content_content_order
  • curriculum_topic_content_id points to question

Okay! && curriculum_topic_content_id shoudl poin to curriculum_topic_content_id in the Curriculum_topic_content table

constraints

  • unique: [course_topic_content_content_order, course_id],
  • unique: name
  • end after start
  • dead after end

Unique per name per course, per unit okay!

course_topic_question

discrepancies

  • webwork_question_ww_path should have table prefix, should it also have ww removed?

We can prefix it. The reason I had for this is the same for the curriculum side.

  • curriculum topic should be renamed to course topic
  • add curriculum question link

Okay!

constraints

  • unique: [problem number, topic]

Okay!

student_enrollment

discrepancies

  • prefix enroll and drop date with table

okay!

constraints

  • drop after enroll

explain?

  • unique: [user, course]
  • application: no professors? shoulda professor enroll in there own class?

Technically no, but what's the harm? Lets ask Drew.

student_workbook

discrepancies

constraints

student_grade

discrepancies

  • need best score (including not counted)

    okay!

constraints

  • latest after first
  • overall best greater than best
tommy-lettieri commented 4 years ago

Right now user role does not exist, it was a permission table that looked like a role table in the past, i think we need to have a discussion on how we want permissions to work

tommy-lettieri commented 4 years ago

Using the diffing method provided above, this is the output, I added comments on missing columns diff.txt

Jhuanderson-Rederly commented 4 years ago

Right now user role does not exist, it was a permission table that looked like a role table in the past, i think we need to have a discussion on how we want permissions to work

Okay! - It works for now. We only have students and professor with roles that would never clash, but down the line certain users can have more than one permission. We can discuss more

Jhuanderson-Rederly commented 4 years ago

Using the diffing method provided above, this is the output, I added comments on missing columns diff.txt

Thanks. I will take a deeper look. I will change the DB so that we can re-run? would that be most helpful?

Jhuanderson-Rederly commented 4 years ago

"users" is plural while the other tables are all singular, I assume this was done to avoid conflicts with internal database tables, can we rename it to something like "system_user" to keep plurality consistent? If not can we change the prepended table name on the fields to users instead of user?

Honestly, I prob debated users vs user for like an hour. We can change it to user or system_user. I like system_user or rederly_user. @tommylettieri

Jhuanderson-Rederly commented 4 years ago

Only some of the tables have last_edited and created_at timestamps, should they all have that?

For the LTI, we are going to need this is a bunch of other places. Let me check if it makes sense to add it to all. I am not seeing any harm of at this moment.

Jhuanderson-Rederly commented 4 years ago

Also can I comment directly after your comments. I have just been quoting. I honestly do not use github for comments and have no idea if I can comment under your comment or if quoting is the only way. haha! @tommylettieri ^^^ think these comments should answer everything for now.

tommy-lettieri commented 4 years ago

Using the diffing method provided above, this is the output, I added comments on missing columns diff.txt

Thanks. I will take a deeper look. I will change the DB so that we can re-run? would that be most helpful?

Yup that would be awesome, I would like to use that DB diagram as a requirements doc, rather than do it the other way around, that way we are thinking about it from the feature rather than the implementation

tommy-lettieri commented 4 years ago

"users" is plural while the other tables are all singular, I assume this was done to avoid conflicts with internal database tables, can we rename it to something like "system_user" to keep plurality consistent? If not can we change the prepended table name on the fields to users instead of user?

Honestly, I prob debated users vs user for like an hour. We can change it to user or system_user. I like system_user or rederly_user. @tommylettieri

Ooo I really like rederly user, do we have to worry about product rename though down the line?

tommy-lettieri commented 4 years ago

For the LTI, we are going to need this is a bunch of other places. Let me check if it makes sense to add it to all. I am not seeing any harm of at this moment.

Yeah I don't think there is any harm per se, just more maintenance, however I think that is valuable info, it might also make sense to add an updated_by field that links to users to see who made the change, and if you wanted to go another level deeper down the rabbit hole you could have an entire audit table to track changes

tommy-lettieri commented 4 years ago

Also can I comment directly after your comments. I have just been quoting. I honestly do not use github for comments and have no idea if I can comment under your comment or if quoting is the only way. haha! @tommylettieri ^^^ think these comments should answer everything for now.

I don't think there is, it would be nice if you could

tommy-lettieri commented 4 years ago

Add university_id to Curriculum Name unique based on university_id Add textbook string to curriculum - add / edit calls

university group might be a thing in the future (for instance for all cuny schools to share their curriculum)

course name should not be unique course_section_code should not be unique course_semester_code should not be unique course_semester_code should become a foreign key to a new a lookup table