KevinKelly25 / LearnSQL

LearnSQL, a website for learning SQL
1 stars 2 forks source link

Move Student Ops to DB #93

Closed cinnaco closed 5 years ago

cinnaco commented 5 years ago

Introduction

This pull request implements functions which allow students and teachers to enroll in classes using LearnSQL.joinClass() and return a list of currently enrolled classes using LearnSQL.getClasses(). A series of testing routines have also been included to measure the success statuses of each function upon execution.

Function Usage

Test Script

Other Notes

KevinKelly25 commented 5 years ago

Thank you for the PR, everything works for me. However, I see a few small issues that need to be fixed before approval.

While this style may or may not be the better style we should leave changes like this to a separate PR where we change all of our comments to this style at once. Of course this should be after we discuss this change. If you think this is something we should do please bring it up in our next meeting.

michaeltorres1 commented 5 years ago

Thanks for the PR @cinnaco. I tested out the PR and everything works as expected with the exception that @kevinKelly25 mentioned that dblink was already added in userMgmt.sql but is not needed.

I also encountered the same issues that @kevinkelly25 mentioned in his review and in addition found some other errors which are listed below:

We should speak as a team about what type of format we should be using for many of the things I mentioned as well as what @KevinKelly25 mentioned as well. And as @KevinKelly25 mentioned please stick to the way we have it formatted now until we have an agreed format to stick to.

With these issues fixed I do believe your PR will be ready for approval.

cinnaco commented 5 years ago

Thanks for the suggestions @KevinKelly25 and @michaeltorres1. Here are the following changes I have implemented based on your feedback:

The following items I have not changed based on the following reasoning:

KevinKelly25 commented 5 years ago

Thank you for the update @cinnaco thank you for exploring that functionality of dblink_exec(). I should have realized that it would fail since you are using select.

I am still seeing many extra lines that are unneeded such as lines 107 and 113 in studentMgmt.sql

There is a user password in the parameter in joinClass() for no reason as it is unused in the function

Concerning the password issue mentioned in the last comment I believe I explained it wrong/misunderstood it at first. I believe you are counting on the password supplied to be already hashed with the bf cipher. I believe this is the wrong approach since for our other functions we do the hash within the function rather then counting on the webapp to do it for us.

However, I do not believe we need to send the admin password in order to get the DB function to work. From the webapps perspective it is only going to be supplied the class and student information. If coming from an admin route it should attach the current users name as the optional parameter. Since the username is being supplied by the webapp I believe it would be safe to just check if the supplied username is an admin without checking password

Using ClassDB.isMember is unnecessary because the user can be checked to be in role classdb_admin in the current database since the roles are the same for the whole server. This would be preferable then to do a cross database query. You should be able to use the same methodology of ClassDB.isMember to get this to work.

As mentioned above for the user password the class join password should be encrypted within the function rather then expecting it to be supplied encrypted.

I do not believe the if statement in line 161 is needed as if the password is null it would fail the in line 168.

The schema qualifiers for the query that starts on line 67 in studentMgmt.sql are unneeded from everywhere except in FROM LearnSQL.Attends INNER JOIN LearnSQL.Class

Also, for the getClasses function I believe you should add an extra parameter to the function, isTeacher which is by default false. Both the teacher and student needs the get Classes function so it makes sense to combine them into a single function since the only difference is the single boolean isTeacher.

cinnaco commented 5 years ago

Thanks again for the feedback. I have made the necessary corrections and additions you mentioned.

michaeltorres1 commented 5 years ago

Thank you @cinnaco for the changes you have made to this PR.

KevinKelly25 commented 5 years ago

@michaeltorres1 's comments seemed to cover most of the issues I saw. I only saw one more which is that user's full name for join class should not be a required parameter for the join class function. Instead it should be retrieved from the userdata_t table.

You are also declaring checkAdminQuery but never using it. Also is setting the isAdmin to false necessary since you are inserting into it before using it?

cinnaco commented 5 years ago

I have made the changes you guys have recommended. I see most of the suggestions made were fragments of code/comments which were missed based on revisions from previous commits since the PR was posted. Since my PR has challenged many stylistic concerns, at some point we should review and agree on a set standard so that such issues will not occur in the future.