Add-Views-core - Githubissues

DASSL / ClassDB

An open-source system to let students experiment with relational data

https://dassl.github.io/ClassDB/

Other

7 stars 2 forks source link

Add-Views-core #159

Closed KevinKelly25 closed 6 years ago

KevinKelly25 commented 6 years ago

Moved views User, Student, Instructor, and DBManager to own file in src/core

smurthys commented 6 years ago

A few suggestions if you wish to do (use addRoleBaseMgmt.sql as a model):

Starting L14, add comments stating script purpose and prerequisites using comparable comments in addRoleBaseMgmt.sql as a model.
Add three blank lines before start of a view definition (or start of a view's introductory comment).
Add start transaction, notice suppression, and commit.
Replace call to hasClassDBRole with an OR expression involving the three IsXYZ functions.

KevinKelly25 commented 6 years ago

@smurthys thank you for the suggestions. I updated the branch to reflect your suggestions.

I didn't edit the comments for each view yet because I felt that may be better left until they are fully written out.

wildtayne commented 6 years ago

Everything looks great so far. Are we planning on adding the missing DDL/Connection activity attributes in this PR? If so, I will hold off on approving.

smurthys commented 6 years ago

IIRC, only the branch add-frequent-views is blocked by this PR. So, I tend to think we should leave this PR open. Any of the maintainers that find time should assign themselves this PR and add the missing view attributes.

Would this work?

wildtayne commented 6 years ago

Yes, I think that will work.

wildtayne commented 6 years ago

The commits pushed merge @KevinKelly25's work in the Winter2018 repo, with a couple tweaks:

Using LEFT OUTER JOIN to keep users which have no activity recorded
All attributes have been added to all core views
Some basic testing has been done, and the views seem to work OK

KevinKelly25 commented 6 years ago

@srrollo thank you for fleshing it out for me, seems we were working on it at the same time in different repos.

I just finished working on a updated winter2018 repo version and made sure it compiles. Seems like we ended up with nearly the same thing. However, I was able to compile the view without schema name qualifier RoleBase in Lns: 59, 65, 71.

Otherwise, it looks great. Thank you for getting it working.

wildtayne commented 6 years ago

Thanks for the verification @KevinKelly25. I just pushed another commit removing the unnecessary qualifications and shortening a few lines. I think this is ready to have some final reviews on it.

afig commented 6 years ago

This looks good, thanks for working on this @KevinKelly25 and @srrollo.

Here's a few comment issues I noticed:

L 3: Missing comma after author's name
L 43-44: Comment is no longer relevant
L 57: preformed -> performed
L 62: amout -> amount

Also, I feel like most of the comments within the definition of the User view are not necessary, since the column names are rather self-explanatory. Not sure if there's some other reason that they are there.

I tested retrieval of students/instructors/DB managers, which worked properly. ~However, I was unable to test display of connection and DDL activity since I do not have a working logging system setup, and it seems that code from what is currently three separate branches is needed to set it up.~

I was able to set up logging and it also displayed without issues.

wildtayne commented 6 years ago

Thanks for the reviews. I've fixed the comment errors pointed out. I will merge this now in the interest of time.