Skip to content

Added getInstructorInfo.sql#32

Merged
smurthys merged 11 commits intomasterfrom
get-instructor-info
Jul 19, 2017
Merged

Added getInstructorInfo.sql#32
smurthys merged 11 commits intomasterfrom
get-instructor-info

Conversation

@zbhujwala
Copy link
Copy Markdown
Contributor

No description provided.

- Finished the Prologue for importOpenCloseCSV.bat
- Changed function call to createOpenCloseStagingTable.sql
- Forced execution of all commands to single transaction
@wildtayne
Copy link
Copy Markdown
Contributor

These look good - I will approve this request once I test these functions with the updated gradebookServer from #30.

@wildtayne
Copy link
Copy Markdown
Contributor

wildtayne commented Jul 15, 2017

I have found one issue with getSections - it seems to return all sections taught by a professor during a term, rather than just sections from the specified term + course combination. Also, it appears that we are moving all .sql files into src/db. Otherwise, these functions should be good to go.

@smurthys
Copy link
Copy Markdown
Member

Following the commit with a note about reverting previous commits, I assume this PR is presently in a suspended state.

Fixed error where `getSections` would return all courses an Instructor
taught
@zbhujwala
Copy link
Copy Markdown
Contributor Author

The previous reverts were due to an error on my end when I used a remote branch as the origin for the get-instructor-info branch. Commit 7f6f76a fixed this issue by changing the origin back to master allowing the appropriate environment for this branch.

@smurthys
Copy link
Copy Markdown
Member

I have the following observations:

createRosterImport.sql:

  • L1: Filename is incorrect
  • L29: Refers to an undefined "rosterStaging folder". I assume this is a reference to table rosterStaging
  • L38: Function name importFromRoster is not schema qualified

getInstructorInfo.sql:

  • Incomplete credit list: Credit people who designed the API/solution implemented in the file and those who made meaningful contributions (such as provide actual code in a comment) even if they did not type in any code in this file. Please see an earlier mail on this topic.
  • Function names are not schema qualified
  • L68-71: Schema name does not begin with upper case: change gradebook to Gradebook.

Qualified functions for 'gradebook' schema
Changed 'Gradebook' to 'gradebook' for some qualified tables and
functions
smurthys
smurthys previously approved these changes Jul 17, 2017
Copy link
Copy Markdown
Member

@smurthys smurthys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for your persistence @zbhujwala.

I recommend merging the changes after approvals from all designated reviewers.

Edit: I dismissed this approval following @afig's request for change and my subsequent comment with sample query to use.

Copy link
Copy Markdown
Contributor

@afig afig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While working on the web client, I came across some issues with the data being produced by these functions.

After taking a look at the code in getInstructorInfo.sql, these issues seem to be the result of improper condition checking. Here is an example query that demonstrates what seems to be the cause of the issues below:

testdb=> SELECT true OR false AND false;
 ?column?
----------
 t
(1 row)

testdb=> SELECT (true OR false) AND false;
 ?column?
----------
 f
(1 row)

In Postgres, AND has a higher precedence than OR, which seems to follow the SQL standard.

Taking a look at the previous comments, it seems that @srrollo had already observed a similar issue in getSections.


  • getSeasons: seems to return all defined seasons regardless of year or wether an instructor has sections in that year, as long as a valid instructor is given:

For instance, the instructor with ID 480 only has 3 sections of CS110 in Spring 2017, and a section of CS170 in Fall 2099 (the latter for testing purposes) yet the following data is returned:

testdb=> SELECT gradebook.getSeasons(480, 2017);
    getseasons
------------------
 (0,Spring)
 (1,Spring_Break)
 (2,Summer)
 (3,Fall)
 (4,Intersession)
(5 rows)

testdb=> SELECT gradebook.getSeasons(480, 2099);
    getseasons
------------------
 (0,Spring)
 (1,Spring_Break)
 (2,Summer)
 (3,Fall)
 (4,Intersession)
(5 rows)

testdb=> SELECT gradebook.getSeasons(4800, 0);
 getseasons
------------
(0 rows)
  • getCourses: similarly, all courses of a given instructor are returned, regardless of year or season
testdb=> SELECT gradebook.getCourses(480, 2017, 0);
 getcourses
------------
 CS110
 CS170
(1 row)


testdb=> SELECT gradebook.getCourses(480, 2017, 9);
 getcourses
------------
 CS110
 CS170
(1 row)


testdb=> SELECT gradebook.getCourses(480, 2070, 0);
 getcourses
------------
 CS110
 CS170
(1 row)

testdb=> SELECT gradebook.getCourses(4800, 2070, 0);
 getcourses
------------
(0 rows)


testdb=> SELECT gradebook.getCourses(4800, 2017, 0);
 getcourses
------------
(0 rows)
  • getSections: all section numbers belonging to sections that an instructor teaches are returned, regardless of year, season, or course
testdb=> SELECT gradebook.getSections(480, 2017, 0, 'CS110');
 getsections
-------------
 (1106,05)
 (1107,72)
 (1108,74)
 (1110,01)
(4 rows)

--similar results to the previous tests when given invalid parameters

Another small yet important issue

  • The getSeasons function is missing a Gradebook. schema qualification when it is being created.

@smurthys
Copy link
Copy Markdown
Member

Good description @afig. Yes, this issue is related to (but also different from) the issue @srrollo described earlier.

In addition to the issue with operator precedence, in general, the queries are missing a join.

Here is the query to use in getSeasons. I have tested it cursorily, but please review/test before implementing. Also, please format all queries as shown in this snippet:

SELECT DISTINCT S."Order", S.Name
FROM Gradebook.Season S JOIN Gradebook.Term T ON S."Order" = T.Season
     JOIN Gradebook.Section N ON N.Term  = T.ID
WHERE T.Year = year
      AND (N.Instructor1 = instructorID
          OR N.Instructor2 = instructorID
          OR N.Instructor3 = instructorID
          )
ORDER BY S."Order";

@smurthys smurthys dismissed their stale review July 17, 2017 10:59

Following @afig's request for changes and my subsequent comment with sample query to use

@smurthys smurthys mentioned this pull request Jul 17, 2017
@smurthys
Copy link
Copy Markdown
Member

I propose approving this PR with the knowledge of the issues documented in this thread. I believe it is OK to merge the changes because as it is this PR provides the correct DB interface to the web server.

If the PR is approved, I will resolve conflicts, merge, and then fix the known issues in a new branch.

@smurthys
Copy link
Copy Markdown
Member

I just resolved conflicts (had three easy-to-resolve conflicts).

@srrollo Please review and indicate approval or not. If this PR gets your approval as well, I will merge and then create a new branch and fix issues.

Copy link
Copy Markdown
Contributor

@wildtayne wildtayne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with merging as-is

@smurthys smurthys merged commit 19351b6 into master Jul 19, 2017
@smurthys smurthys deleted the get-instructor-info branch July 19, 2017 17:12
@smurthys
Copy link
Copy Markdown
Member

Created Issue #36 to fix known issues in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants