Updated gradebookServer.js DB function names to match #45 + Misc. fixes

[wildtayne]

This pull request fixes a couple items in gradebookServer.js. This PR updates some function names based on changes in #45, so this PR must be merged after #45.

• Instructor REST calls now use the updated DB function names (From Completed milestone M1 tasks related to instructor management #45)
• The login failed message now sends the correct error code (401)
• Added a check for blank attendance sheets - if no records are found, a 500 error is sent instead of the server crashing
• Updated the attendance function to use the correct column name from the DB

[smurthys]

The changes look good.

A minor point: The comment on L138 should probably be stated more like the comment on L299 (because the function will not have returned any row).

[smurthys]

L300 (was L299) has the correct comment (but for the spelling error in the last word): //Check if any attendance data was retreived

The comment on L138 //Check if the returned row contains a null instructor id is literally correct, but is semantically incorrect, because the check there is actually to see if the result contains a row at all.

[afig]

Overall, changes look good. However the new check on line 139, if(result.rows[0].id == null), seems to have introduced a larger issue. Calling /login with an invalid email causes the server to crash with the following error:

      if(result.rows[0].id == null) {
                       ^

TypeError: Cannot read property 'id' of undefined

It seems like the check relies on receiving a table with (at least) 1 row with (at least) NULL in the id column. However, this does not appear to be a possible return value of calling the gradebook.getInstructor(email) function. The function in question appears to return a table with 0 rows if it does not locate an instructor.

I am not too familiar with the behavior of the pg module we are using, but it seems like result.rows is an empty array if the query did not return any rows, which appears to be causing the above error. Instead, it might be a better idea to check that result.rowCount is > 0. It might even be optimal to check this for all queries, or at least catch any error that might result from attempting to read a result with no rows.

[smurthys]

Thanks @afig for sharing your observation on handling a result set with no rows. I did question the current implementation in my own mind, but chose to not ask for I wasn't aware of the details of the pg module.

This kinds of situations is why I like at least two approvals to every PR and like every review to be careful and analytical. This attitude is also why and how teams fare better.

So, a big thank you to all contributors who are making this development such an enjoyable and educational effort.

[wildtayne]

Thanks for the review. I just pushed a commit that changes the approach when checking for returned data from the getAttendance() and getInstructor() to check the number of rows returned. For getInstructor(), I choose to check if the result cardinality is exactly 0, since getInstructor() can return only 0 or 1 rows.
For getAttendance(), I check if the result cardinality is exactly 1, since the header row is always returned by getAttendance().

I agree that a similar check should be added to all the REST functions, however I think we may want to consider how the functions behave when no data is returned. Currently, /attendance sends a 500 error if no attendance records are available, which causes an error to be displayed on the client. I think this is not correct, as no attendance records existing for a section is not an exceptional situation. I think it makes more sense to send a different code for no data (maybe 204) and have the client display a message that no records exist in the attendance grid area. I think this is the ideal behavior for all of the current REST functions except for /login, which is correctly sending a 403 error.

I can integrate this change into this branch, I can create an issue and address it after M1.

[wildtayne]

I also improved the comments related to the return checking and made them reflect the current implementation.

[smurthys]

Nice work @srrollo. These changes look good to go.

[smurthys]

Sorry, I again forgot to select the Approve option in my last attempt.

[afig]

Changes look good, thanks.

As far as checking for all queries goes, I think it would be best to add an issue and perform the changes after M1, considering the approaching deadline and the possibility of further changes to the return value of functions.