DASSL / ClassDB

An open-source system to let students experiment with relational data
https://dassl.github.io/ClassDB/
Other
8 stars 2 forks source link

Add a rough draft of ERD #139

Closed KevinKelly25 closed 6 years ago

KevinKelly25 commented 6 years ago

Adds a ERD for the detail tables mentioned in issue #138

smurthys commented 6 years ago

Thanks @KevinKelly25 for getting this started.

A few things to consider:

I wonder what people think about the naming convention for attribute names in tables: Thus far, ClassDB has made first-letter lower case, whereas Gradebook makes first-letter upper case. Which approach should we use?

I prefer first-letter upper case because that seems to be the industry norm, and it distinguishes attributes from variables in code.

KevinKelly25 commented 6 years ago

Added corrections as discussed in today's meeting and issue #138 .

afig commented 6 years ago

Thanks for making the changes @KevinKelly25, they look great.

I have a few minor suggestions:


In terms of naming conventions for attribute names in tables, I do not have a strong personal preference between the two major conventions. However, since first-letter-uppercase appears to be more common overall, then I agree that we should consider switching. If we agree to switch, then a separate issue should be opened.

wildtayne commented 6 years ago

Overall, the latest ER schema looks good.

One additional observation I have is that StatementSuceededAtUTC is not the best name for this attribute. Currently, we are using statement_timestamp to determine the timestamp for DDL activity. According to the Postgres docs, this returns the timestamp when the Postgres server received the current statement from the client. Perhaps StatementStartedAtUTC would be a better name.

I also agree that beginning attribute names with capital letters is a better convention, and that this should be addressed in a future issue.

KevinKelly25 commented 6 years ago

Thank you @afig and @srrollo for the comments. I fixed the issues you brought up. I did leave the attributes in the current system we have, lower case first. However, I also agree that switching to what is the industry norm according to @smurthys is probably a good idea.

smurthys commented 6 years ago

Good changes @KevinKelly25

A few observations:

smurthys commented 6 years ago

I have added the Issue #140 about letter case. Please discuss that specific issue over there.

Because the ER Schema needs to be edited, it is OK if the issue is fixed in this schema. However, I am also OK if it is addressed in the next round (because we know this schema will soon be outdated). I'd prefer if this PR is approved and merged before this afternoon's call (but of course only without sacrificing quality).

KevinKelly25 commented 6 years ago

Updated ERD with corrections for @smurthys comments and used new standard for attribute names discussed in issue #140

KevinKelly25 commented 6 years ago

Updated the Attribute box to include authors with meaningful contributions as per the discussion in the meeting today.

smurthys commented 6 years ago

Good work @KevinKelly25.

After all reviewers have approved, please hit the "Merge pull request" button to merge your changes to master. I believe the changes should merge without any conflict.

Remember to delete your branch after the merge is complete and you have verified the file appears in the master branch.

afig commented 6 years ago

Also of note, when merging a pull request, there are three options, which can be seen by clicking the drop-down arrow next to the merge button. The three options are:

So far, we have been using the "Create a merge commit" option, and thus this Pull Request should also do the same. "Create a merge commit" should be the default options, but is worth checking. If you wish, you can read more about the differences between each method here.

KevinKelly25 commented 6 years ago

Thank you, once approved by @baconbm I will do the create a merge commit