Honor timezone of the acting library where appropriate

Bug #1705524 reported by Mike Rylander on 2017-07-20
26
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Evergreen
Wishlist
Unassigned

Bug Description

This is a followup to the work done in bug 1485374, where we added the ability for the client to specify a timezone in which timestamps should be interpreted in business logic and the database.

Most specifically, this work focuses on circulation due dates and the closed date editor. Due dates, where displayed using stock templates (including receipt templates) and used for fine calculation, are now manipulated in the library's configured timezone. This is controlled by the new 'lib.timezone' YAOUS, loaded from the server when required. Additionally, closings are recorded in the library's timezone so that so that due date calculation is more accurate. The closed date editor is also taught how to display closings in the closed library's timezone. Closed date entries also explicitly record if they are a full day closing, or a multi-day closing. This significantly simplifies the editor, and may be useful in other contexts.

To accomplish this, we use the moment.js library and the moment-timezone addon. This is necessary because the stock AngularJS date filter does not understand locale-aware timezone values, which are required to support DST. A simple mapper translates the differences in format values from AngularJS date to moment.js.

Of special note are a set of new filters used for formatting timestamps under certain circumstances. The new egOrgDateInContext, egOrgDate, and egDueDate filters provide the functionality, and autogrid is enhanced to make use of these where applicable. egGrid and egGridField are also taught to accept default and field-specific options for applying date filters. These filters may be useful in other or related contexts.

The egDueDate filter, used for all existing displays of due date via Angular code, intentionally interprets timestamps in two different ways WRT timezone, based on the circulation duration. If the duration is day-granular (that is, the number of seconds in the duration is divisible by 86,400, or 24 hours worth of seconds) then the date is interpreted as being in the circulation library's timezone. If it is an hourly loan (any duration that does not meet the day-granular criterium) then it is instead displayed in the client's timezone, just as all other timestamps currently are, because of the work in 1485374.

The OPAC is adjusted to always display the due date in the circulating library's timezone. Because the OPAC displays only the date portion of the due date field, this difference is currently considered acceptable. If this proves to be a problem in the future, a minor adjustment can be made to match the egDueDate filter logic.

This work, as with 1485374 was funded by SITKA, and we thank them for their partnership in making this happen!

Branch following soon...

Mike Rylander (mrylander) wrote :
tags: added: pullrequest
Changed in evergreen:
assignee: Mike Rylander (mrylander) → nobody
Mike Rylander (mrylander) wrote :

I've added a commit to enhance the grid's ability to autoformat dates, and to use grid formatting on the patron bills list UI.

Mike Rylander (mrylander) wrote :

Removing pullrequest from this while I address the upgrade process a bit more...

tags: removed: pullrequest
Mike Rylander (mrylander) wrote :

Upgrade script adjusted. From the final commit:

Now that due dates are globally stored in the configured timezone of the circulating library, the automatic adjustment to day-granular due dates needs to take those timezones into account.

An optional SQL command is provided by the upgrade script to retroactively adjust existing due dates after library configuration is complete.

tags: added: pullrequest
tji@sitka.bclibraries.ca (tji) wrote :

I have tested this code and consent to signing off on it with my name, [Tina Ji] and my email address, [<email address hidden>].

tji@sitka.bclibraries.ca (tji) wrote :

I have tested this code and consent to signing off on it with my name, Tina Ji and my email address, <email address hidden>.

tags: added: signedoff
Mike Rylander (mrylander) wrote :

I've pushed a new branch with Tina's signoff. For testing, my primary concern is the complete correctness of the post-upgrade optional change to existing circs. I would appreciate testing of that, especially repeated runs against hourly circs when an org unit's timezone is different from the DB server's timezone.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=a396a20a5204da34497c1555f29e740f3542ba8c

Mike Rylander (mrylander) wrote :

Updated the branch with a commit that overrides the built-in angular date filter with a moment-js based one. This provides consistency and proper timezone support.

Bill Erickson (berick) wrote :

Grabbing to review code and do some more testing. For now I'm holding off on testing the optional post-upgrade change, hoping someone w/ hourly circ data can can look at that.

Changed in evergreen:
assignee: nobody → Bill Erickson (berick)
Bill Erickson (berick) wrote :

Light functionality testing all looks good, minus issues noted below. Seeing the due time for hourly checkouts is pretty great! I also ran the optional post-upgrade SQL one time through on a modified TZ (BM1=America/Los_Angeles) and it behaved as expected for hourly circs.

New branch pushed:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1705524-lib-tz-review

1. Adds sign-off for Mike's last commit overriding the date filter.

2. Fixes a bug in the closed dates editor where it failed adding dates for branches with no org time zone setting. (null ref)

3. Adds a commit to load the timezone setting of the workstation org unit during startup. This is an optimization based on the assumption that the timezone of the workstation org will likely be loaded frequently, in which case this saves us an API call by bundling the lookup with an existing batch org setting call.

4. Adds a commit to s/undef/null in some JS code. (probably squash this).

Issues I did not address in my branch:

1. The release notes are a lot to digest for the casual administrator / end user. I'd love to see a short bullet summary at the top listing the user-facing changes. A note on how to find a list of valid timezone values for the new setting would also be appreciated.

2. The SQL \q echos are gobbling single quotes (and warning about unterminated strings) leading to a bogus optional query. My preference would be to move the optional SQL to the release notes for safe keeping.

3. I get the following console error when (accidentally) checking out an item (owned by BR1, no timezone setting) that's already checked out:

https://docs.angularjs.org/error/$parse/ueoe?p0=circDate%20%7C%20date:

4. I get this console error checkout out a precat item:

Deprecation warning: value provided is not in a recognized RFC2822 or ISO format. moment construction falls back to js Date(), which is not reliable across all browsers and versions. Non RFC2822/ISO date formats are discouraged and will be removed in an upcoming major release. Please refer to http://momentjs.com/guides/#/warnings/js-date/ for more info.
Arguments:
[0] _isAMomentObject: true, _isUTC: false, _useUTC: false, _l: undefined, _i: now, _f: undefined, _strict: undefined, _locale: [object Object]
Error
    at Function.createFromInputFallback (https://eg-dev-local/js/ui/default/staff/build/js/moment-with-locales.min.js:1:3248)
    at gb (https://eg-dev-local/js/ui/default/staff/build/js/moment-with-locales.min.js:1:19736)
...
    at https://eg-dev-local/js/ui/default/staff/services/ui.js:129:17
...

The last 2 did not create user-facing problems as far as I could tell.

Removing 'signed-off' tag pending feedback.

Thanks Mike!

tags: removed: signedoff
Mike Rylander (mrylander) wrote :

Thanks, Bill. I'll work on the 4 things you identified.

Mike Rylander (mrylander) wrote :

I've pushed a branch with cross-signoffs for Bill's commits, and a commit to address the four points he raised in comment #10.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp1705524-lib-tz-review-cleanup

Bill Erickson (berick) on 2017-08-11
Changed in evergreen:
status: New → Confirmed
Bill Erickson (berick) wrote :

Thanks for the extras, Mike. Looks good. Pushed to master.

Changed in evergreen:
status: Confirmed → Fix Committed
assignee: Bill Erickson (berick) → nobody
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers