Action trigger Granularity strings saving label not value

[Kevin Reed]

Evergreen 2.4

Open-ILS/src/templates/conify/global/action_trigger/event_definition.tt2

<div class='hidden'>
    <select dojoType='dijit.form.ComboBox' jsId='eventDefGranularity'>
        <option value='hourly'>[% l('Hourly') %]</option>
        <option value='daily'>[% l('Daily') %]</option>
        <option value='weekly'>[% l('Weekly') %]</option>
        <option value='monthly'>[% l('Monthly') %]</option>
        <option value='yearly'>[% l('Yearly') %]</option>
    </select>
</div>

when saving the action trigger the string with the capital first letter (eg: "Daily") is saved to the database not the lower case value string.

The action trigger cron job, when specifying granularity takes capitals into account. Therefor action triggers with granularity set made after an upgrade will not work with the cron job set up for previous versions of evergreen.

Simple solution, change the strings to be lower case. Proper fix: have the value saved not the label.

[Pasi Kallinen (paxed)]

Also: If the UI is used in some other language than English, the granularity will be the translated string.

[Bill Erickson (berick)]

Fixes pushed to:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1205072-action-trig-granularity-ui

From the commit:

Present A/T granularity options as untranslated, lower-case strings to match the crontab examples. Also, honor alternate case variations for granularity values so that "Daily" and "daily" are both separate, valid options in the UI.

[Josh Stompro (u-launchpad-stompro-org)]

I just tried out the fix and it worked great on a 2.7.1 system. I added in "weekdays" also just to see if it was that easy. Since the example cron includes a weekday action trigger granularity run, maybe that should be include also.

If this goes through, should there be a note made in the release notes, this change could cause problems for someone if they start creating events with the lowercase granularity names by selecting from the dropdown, when their cron jobs are setup to run the capitalized name.

Josh

[Bill Erickson (berick)]

Agreed on upgrade notes. I've pushed some to the same branch. Sanity check appreciated.

[Remington Steed (rjs7)]

The Docs Interest Group currently uses the release notes to guide related updates to the public documentation, but this is not true for updating the help text in the action_trigger_runner.pl script. We should add a commit to this branch which changes the help text example from "granularity=Hourly" to "granularity=hourly" (as suggested by Josh in https://bugs.launchpad.net/evergreen/+bug/1396161/comments/3).

[Josh Stompro (u-launchpad-stompro-org)]

Here is a branch with "weekdays" added as an option to match the example crontab file.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/stompro/lp1205072-action-trig-granularity-ui

Here is a branch that changes action_trigger_runner.pl back to a lowercase hourly. It also includes a commit with changes to the docs wherever I could find uppercase granularity values mentioned. (I didn't touch the instance in the Telephony doc pages, I plan on working on those later anyway."

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/stompro/lp1396161_action_trigger_runner_docs_v2

[Dan Wells (dbw2)]

Tested by Remington and me, looks fine. Pushed back through rel_2_6. Thanks, Bill, Josh, and Remington!

As a note, I also backported the release notes file along with the rest. I know we have no current policy to do anything regarding release notes for minor versions, but saw no harm in including it. That said, maybe this change shouldn't be backported at all? Any opinions on that?

[Josh Stompro (u-launchpad-stompro-org)]

What would the exact scenario of breakage be? Someone upgrades to 2.7.4. They have their cron jobs referencing the uppercase granularity settings. Then they go in and make changes to an event definition, which saves the new lowercase granularity label. Now their events won't get processed until they adjust their cron jobs and they would also need to edit all the event definitions that use granularity so some are not on the old format and some on the new.

This has to happen at some point for this change to go through. If it doesn't happen when someone upgrades to the next point release, then it will happen when they do a major upgrade. It probably is bad form to do this for minor upgrades, when someone would reasonably expect things to just keep working. But they will just keep working right up until you edit your event definitions.

It probably should just go into 2.8 at this point, since 2.8 is so close.

[Dan Wells (dbw2)]

In conversation on IRC, it was concluded that the fix as it stands may cause confusion, and that changing 'granularity' to be entirely case-insensitive would be a potential remedy.

This code has been reverted pending work on the move to case-insensitivity.

[Josh Stompro (u-launchpad-stompro-org)]

Hello, would this be the correct approach to make the granularity setting case-insensitive?

Open-ILS/src/perlmods/Lib/OpeILS/Application/Trigger.pm

I think this is where the granularity comparison is done, starting at line 650.

 if (defined $granularity) {
if ($granflag) {
$query->[0]->{'+atevdef'} = {granularity => $granularity};
} else {
$query->[0]->{'+atevdef'} = {'-or' => [ {granularity => $granularity}, {granularity => undef} ] };
}
} else {
$query->[0]->{'+atevdef'} = {granularity => undef};
}

Can I just add lower and lc like below to force both the database and the submitted granularity value to be evaluated as lower case? Is there a better method through the field mapper to always force granularity to be lower case?

650 #Remove case from granularity by forcing to lower case.
   651 if (defined $granularity) {
   652 if ($granflag) {
   653 $query->[0]->{'+atevdef'} = {lower(granularity) => lc $granularity};
   654 } else { #not currently used since $granflag is always set if $granularity is set
   655 $query->[0]->{'+atevdef'} = {'-or' => [ {granularity => $granularity}, {granularity => undef} ] };
   656 }
   657 } else {
   658 $query->[0]->{'+atevdef'} = {granularity => undef};
   659 }

Thanks
Josh

[Mike Rylander (mrylander)]

Josh,

You can't use the syntax you suggest directly, however you can do what you want. It would be something like:

$query->[0]->{'+atevdef'} = {
  granularity => {
     '=' => {
      transform => 'evergreen.lowercase',
      value => lc($granularity)
    }
  }
};

The has inside the array of the -or branch would need to be changed likewise. See Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Util.pm line 837 (in master) for an example.

HTH.

[Josh Stompro (u-launchpad-stompro-org)]

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/stompro/lp1205072-action-trig-granularity-ui-case-insensitive

Here is a branch that tries to make the granularity setting case insensitive. It works well in my testing, HOURLY and HoUrLy are treated the same, and the events get generated properly.

Josh

[Josh Stompro (u-launchpad-stompro-org)]

Bill, do you think your changes to the GUI should still be included if the granularity value is no longer case sensitive? (minus the release notes since the situation described there shouldn't be an issue anymore.)

My thoughts are that your change is valuable because it makes the strings untranslated, so they won't be changing for different locales.

I forced pushed to my existing branch an update that squashes my commits together and changes the release notes.
user/stompro/lp1205072-action-trig-granularity-ui
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/stompro/lp1205072-action-trig-granularity-ui

Josh

[Ben Shum (bshum)]

Not sure how this bug stands at the moment, switching its focus to 2.next for now and removing milestones. At one point we weren't sure if this was a new feature approach (only to .next, and not to be backported).

Will review again at at a later date.

[Kathy Lussier (klussier)]

The new feature vs. bug discussion occurred when there were concerns about existing cron jobs not working due to the change in case. I think those concerns are now addressed because the new approach is to make the granularity settings case insensitive. I vote for bug fix.

[Kathy Lussier (klussier)]

I finally got around to loading this on a test VM with a fairly recent checkout of the master branch. With the branch loaded, I'm getting an Internal Server error when I navigate to the OPAC or to the web client.

I'm removing the pullrequest tag for now and adding a needsrepatch tag.