DASSL / Gradebook

Open-source product to provide a practical means for instructors to record student attendance and assessment
Other
8 stars 4 forks source link

Simplify attendance mgmt #64

Closed smurthys closed 7 years ago

smurthys commented 7 years ago

The commits in this PR simplifies and somewhat optimizes, as well as fixes some issues in addAttendanceMgmt.sql. Minor issues such as inconsistency, etc. still remain.

To use the changes, the following updated scripts need to be run in the order shown: addSeasonMgmt.sql, addSectionMgmt.sql and addAttendanceMgmt.sql.

Summary of changes

smurthys commented 7 years ago

Thanks @afig for spotting the issues.

Commit 2fa0c0d addresses the issues @afig pointed out, plus:

smurthys commented 7 years ago

Commit f622919 improves function getScheduleDates:

wildtayne commented 7 years ago

Overall, the changes make a big improvement to the Attendance, Season, and Section management code. I did find two related issues with the expanded use of season identification.

Thinking about it, the root of this issue is that Postgres will not coerce a numeric type to a text type in a function's input. A trivial example of this is the function:

CREATE FUNCTION myfunc(VARCHAR(10))
RETURNS VARCHAR(10) AS
$$
SELECT $1
$$
LANGUAGE SQL;

So SELECT myfunc(0::VARCHAR(10)) works, but SELECT myfunc(0) does not. I'm not sure if this requires any action, but we will at least want to be sure that functions supplying a season order supply it as a string.

smurthys commented 7 years ago

Thanks @srrollo for spotting the issues. I have addressed them in new commits:

Commit b97192c: In addSeasonMgmt.sql and addSectionMgmt.sql, added or restored functions which accept season order to support clients that pass that data as a number. This seems like the right and safe thing to do, because it is natural for people to pass a number if they have a number.

Commit cc5b1cd: In prepareRosterImport.sql, removed the call to getSeasonOrder

Commit f7ee798: Unrelated to the original purpose of this PR, but set client_min_messages to WARNING in prepareDB.psql so the scripts executed in the batch do not show unnecessary messages. A few (new) messages still show, but many of the ones that used to show are now suppressed.

Commit 21031e8: Also unrelated to the original purpose of this PR, but fixed an issue in addEmailByInstructorName.sql that resulted in assigning a duplicate email address causing a script failure. While there, also improved the chance of email assignment for instructors without an address.

smurthys commented 7 years ago

BTW, the various README files to install and import data have been very helpful in testing the changes. It is amazing how many details I have already forgotten since M1 was closed. The README files definitely saved me quite a bit of time.

smurthys commented 7 years ago

Thanks @srrollo.