Cost history enhancements

[nkuitse]

I've broken ajax_*.php down into separate files, one for each case in the three big switch statements.

[nkuitse]

Please feel free to reject this pull request -- I'm a git novice and have probably made a mess of things by mixing the code I'm working on to implement cost history tracking for our libraries with the Ajax-related code refactoring (in which I broke out each case in ajax_{forms,htmldata,processing}.php to a separate file). I would be happy to supply a patch against your master branch if that works better for you and you're interested in picking up the refactor.

[remocrevo]

Thanks for your work on this. Is all of this code ready for testing and adding to the master repository? Your comment suggests that the cost history tracking code is still in progress. If that code is not ready for publishing, then please submit a new pull request containing just the refactored code, as you suggested. Again, thanks for contributing!

[nkuitse]

As you can see, I've made a lot more changes. The code is pretty stable now, though; we're running it in a "sandbox" instance and it seems to be working just fine. Note: When I created the pull request I targeted your master branch but in retrospect that's a poor choice.

[benheet]

We've passed our two week window on this one. Are we ready to merge it in?

[jeffnm]

I have not tested this, but if it's passing other people's tests, then merge away. There seem to be some conflicts in the pull request though, so someone should probably look into that.

[benheet]

Paul H., can you review and address the conflicts that Jeffrey mentioned?

[jeffnm]

We are testing this after all. I've installed it on our test sever and our e-resources librarian will be taking a look at it soon. I'll report back any issues we encounter.

[PaulPoulain]

After applying this patch & updating the database:

• I loose existing acquisition types. Maybe it was expected, but it's a pain, I had to re-enter them. On a live install, that would be a big problem
• I can now enter a resource. With the parameter set to Y I see the ENH cost screen:
  

There are 2 problems though:

• the dates are in US format : february 1st is displayed 2/1/2015 (which is unreadable for most of the world ;-) )
• when you want to enter more than one line, the new lines have the date-picker not working : only the 1st datepicker work well (the one at the bottom)

And one question/comment : the % line is not very clear. I entered the following datas:

I expected the % to be calculated only on the "same" line of "last year", ie the same fund/same type line (70€ became 100€ the year after. Instead, the +71 mean that the total cost of 2015 is 71% more than the total cost of 2014)
I think that we should split this % increase in another table, for a better clarity. Like:
2014: 100 n/a
2015: 220 +71.4%

[PaulPoulain]

Note for other testers, here is how I tested this patch.
starting from a fresh 1.2 release (retrieved from coral github):
cd $CORAL/resources

adding another remote source:

git remote add flo https://github.com/fenway-libraries-online/coral-resources.git
#fetching it
git remote update
#checkout the branch flo
git checkout flo
(git log show me that there are patches from paul@flo.org, fine)

preparing the DB update and running it

perl -i -pe "s/DATABASE_NAME/coral_resources_prod/g" install/protected/update_1.2_flo.sql
mysql coral_resources_prod < install/protected/update_1.2_flo.sql
(no error output, nice)

updating configuration

vi admin/configuration.ini
adding enhancedCostHistory=Y line

restarting apache:

/etc/init.d/apache2 restart

[PaulPoulain]

Another problem : the DB update. It's too complex, without reason.
The lines containing things like
ALTER TABLE _DATABASE_NAME_.Resource
must be changed to be
ALTER TABLE Resource

the DB name will be provided when the file is run on mysql:
mysql -u login -p password CORAL_DB <update.sql, so the DATABASE_NAME is useless

[nkuitse]

How do I see the conflicts?

[nkuitse]

Dates in US format area silly oversight on my part. :-( Display date format should IMO be a config file setting. I'll look into that.

[nkuitse]

That's bizarre that you lost acquisition types. I'll fix that. I'll also remove the spurious DATABASE_NAME. bits.

[nkuitse]

Finally, the percent increase could be implemented in different ways; we chose to make it reflect the change in total expenditures from year to year, regardless of fund. However, when we presented the enhancement to our own internal customers -- the ERM librarians who worked on the spec -- they decided that they didn't want a percentage increase display after all, because they found it distracting and unhelpful. So perhaps the solution is to drop it from the enhancement.

[PaulPoulain]

Le 20/02/2015 22:56, Paul Hoffman a écrit :

Dates in US format area silly oversight on my part. :-( Display date
format should IMO be a config file setting. I'll look into that.

I think that there's PHP way of knowing the system format date that
could help: date_default_timezone_set. Isn't there an option in the date
controler about that ?

PS : when coming into date problems, the best solution is usually to
deal everything in ISO internally (YYYY-MM-DD), and switch to local
format only when displaying.

Paul Poulain, Associé-gérant / co-owner
BibLibre, expert du logiciel libre pour les bibliothèques
BibLibre, Open Source software for libraries expert

[PaulPoulain]

Le 20/02/2015 23:01, Paul Hoffman a écrit :

Finally, the percent increase could be implemented in different ways;
we chose to make it reflect the change in total expenditures from year
to year, regardless of fund. However, when we presented the
enhancement to our own internal customers -- the ERM librarians who
worked on the spec -- they decided that they didn't want a percentage
increase display after all, because they found it distracting and
unhelpful. So perhaps the solution is to drop it from the enhancement.

I agree with the idea of dropping it for now, and maybe have another
board with statistic numbers to see how things evolve over time.

Paul Poulain, Associé-gérant / co-owner
BibLibre, expert du logiciel libre pour les bibliothèques
BibLibre, Open Source software for libraries expert

[nkuitse]

Could someone please tell me how to view the conflicts in this pull request? I haven't been able to figure this out.

[jeffnm]

Here's some info about resolving conflicts https://help.github.com/articles/resolving-a-merge-conflict-from-the-command-line/

[nkuitse]

That info is for committers; it doesn't help me.

[jeffnm]

The commit conflicts that are preventing an automatic merge are what I was
referring to previously. If there is some other conflict, I'm not sure how
to help with that.

On Tue Feb 24 2015 at 2:57:22 PM Paul Hoffman notifications@github.com
wrote:

That info is for committers; it doesn't help me.

—
Reply to this email directly or view it on GitHub
#30 (comment).

[nkuitse]

Right -- and thanks for pointing me to the info you found -- but I don't know how to see which commit conflicts are preventing an automatic merge. I'm guessing that I need to clone the master repo anew, then attempt to merge my commits, but I can't quite get my head around how to do that.

[jeffnm]

I see what you mean. I guess changes were made on the master branch between
the time you checked it out originally and your pull request.

This page shows you the commits that the Master Resource module has
received since you forked it.
fenway-libraries-online:flo...ndlibersa:master

This page shows the commits and changes you've made on your fork that
aren't in the master yet.
fenway-library-organization/coral-resources@ndlibersa:master...master

Anywhere the same lines are changed or a file has been deleted will need to
be manually approved. This can be done by the person who does the final
merge, or you can pull the changes from the ndlibersa:master branch into
your branch, fix any problems, and then make another pull request.

I hope that helps.

On Tue Feb 24 2015 at 4:08:33 PM Paul Hoffman notifications@github.com
wrote:

Right -- and thanks for pointing me to the info you found -- but I don't
know how to see which commit conflicts are preventing an automatic merge.
I'm guessing that I need to clone the master repo anew, then attempt to
merge my commits, but I can't quite get my head around how to do that.

—
Reply to this email directly or view it on GitHub
#30 (comment).

[nkuitse]

I have fixed the conflicts and successfully rebased onto the latest HEAD (commit 357fe9f). Or at least I believe that is so; GitHub still says that the pull request contains merge conflicts. (Do I need to submit a new pull request???) I have also fixed the problem with the DB upgrade SQL. Could someone please check to see if this looks OK before I proceed with fixing the date problem and deleting the percent change code?

Finally, I would feel better having this enhancement committed onto a separate branch and then merged into master; is there some reason not to create a branch for significant changes such as this?

[remocrevo]

I'd suggest that any major development should be merged by hand, not by using GitHub's auto-merge feature. You could comment on the GitHub issue and add a link to your latest branch. Then one or two people can test your branch and comment on the issue that they approve (or any concerns they have). When a few people are happy with it, someone can pull your branch to their own machine, merge it with master, and push it to the main GitHub repository. At that point it is considered "released" to the community, so we would need to change the version number, provide information about installing and using the changes, announce to the community, etc.

Does that sound like a reasonable workflow to everyone?

[nkuitse]

That sounds good to me. Here's a link:

https://github.com/fenway-libraries-online/coral-resources

The latest branch is master.

[nkuitse]

I'm looking at the SQL code for upgrading to 1.1 and 1.2 (install/protected/update_1.1.sql and install/protected/update_1.2.sql) and they both have _DATABASE_NAME_ (in backticks) prepended to every table name. The upgrade script (install/update.php) substitutes the actual database name for each occurrence of this placeholder. So IMO this should stay in the DB upgrade code for 1.3.

[nkuitse]

DATABASE_NAME is not useless; the upgrade script uses it:

//Process the sql file by statements
foreach ($statements as $statement) {
   if (strlen(trim($statement))>3){
        //replace the DATABASE_NAME parameter with what was actually input
        $statement = str_replace("_DATABASE_NAME_", $installer->getDatabaseName(), $statement);

        $result = mysql_query($statement);
        if (!$result){
            $installer->addErrorMessage(mysql_error() . "<br /><br />For statement: " . $statement);
            break;
        }
    }
}

[remocrevo]

From my quick investigation, it looks like the DATABASE_NAME prefix could be removed from the SQL file without causing harm to the installation/upgrade process. But I would want someone to test that. In general, I think there are better ways for accessing a specific database, such as the mysql_select_db() function that CORAL already uses. So I am happy for us to remove the prefix and the code that uses it. But that is not a necessary change right now, and it would slow down the process of releasing this code. I recommend that @PaulPoulain create a github "issue" for that change so we can address it separately, and if it is causing a lot of trouble for testing the db scripts, we can address it quickly.

[nkuitse]

I've been going around in circles trying to fix the date picker bug that Paul Poulain spotted in the "Edit cost history" box. I figured out that I have to re-add date pickers when the user clicks on the "Add" button, so that the added row will get a date picker for each of its two date fields:

$(".addPayment").live('click', function () { 
    // 150+ lines omitted
    $('.date-pick').datePicker({startDate:'01/01/1996'});
    return false;
}

And indeed when I make that change works, in a way -- it adds a date picker to every date field, which means that the date fields that already existed on the page now have two date pickers. :-(

So I tried this, which should (I think) remove the date pickers; now the code looks like this:

$(".addPayment").live('click', function () { 
    // 150+ lines omitted
    $('.dp-choose-date').remove();
    $('.date-pick').datePicker({startDate:'01/01/1996'});
    return false;
}

And indeed it does that, sort of -- the date pickers are removed, and the only date pickers are in the newly added row.

Any ideas how to fix this? Should we just go ahead and merge the pull request with this bug present?

[nkuitse]

Never mind, I figured it out. The fix is a hack, though -- it requires duplicating some code from jquery.datepicker, because the version we're using doesn't have the functionality we need. Here's the final addition just FTR; I'll commit the change in a minute:

// Remove all date pickers
$('.dp-choose-date').remove();
// HACK: clear the internal ID
$('.date-pick').each(function() {
        this._dpId = 0;
});
// HACK: clear the cache (may not be necessary)
var els = $.event._dpCache || [];
for (var i in els) {
        $(els[i].ele)._dpDestroy();
}
$.event._dpCache = [];
// Re-add all date pickers
$('.date-pick').datePicker({startDate:'01/01/1996'});
return false;

[nkuitse]

Remington, are you OK with merging this now?

[remocrevo]

Duplicate word "your your", one should be removed.

[remocrevo]

Hm, the date picker fix does look pretty hacky. I'm impressed that you figured it out, but I think it's best if we can find a better way. So my colleague and I installed this branch, tested it, and found a slightly simpler fix. Tell me what you think. You should be able to replace your whole change with the following four lines:

// Remove datepickers from clone
originalTR.find('.dp-choose-date').remove();
// Re-add datepickers to clone
originalTR.find('.date-pick').datePicker({startDate:'01/01/1996'});

Looking into it a little more, it seems we are on a very old version of jQuery, and an even older version of the date picker plugin. Ultimately, we need to upgrade CORAL to a modern version of jQuery and make sure we keep up with new versions in the future. But for now, I will create a separate issue for that.

[nkuitse]

I've committed your revised fix. I agree on postponing a jQuery upgrade for now, FWIW.

[remocrevo]

Also, regarding @PaulPoulain 's problem of losing acquisition types, maybe that was a misunderstanding? In our testing, we didn't lose those values, but we noticed they were moved to a separate edit form. And the new cost history form contains an empty "Cost Details" dropdown which might have been the cause of confusion. @PaulPoulain does that explain the problem you had? It might help to include some default values with this branch. I'm not really sure what the intended use of that dropdown is. @nkuitse can you comment on that and/or supply some default entries for the CostDetails table?

[nkuitse]

The spec we worked up for the enhancement says the new CostDetails table "provides a controlled entry list for use at the institution's discretion" and my reading of things is that it's a way to give people a flexible (but controlled) way to categorize subscriptions or payments that is orthogonal to the payment type and fund, and that avoids shoehorning stuff into free-text notes. The only examples given in the spec are "FTE", "FTE 25", and "FTE 26". I've asked Kelly for clarification in case she has any additional insights -- she was deeply involved in the writing of the spec, which I was not.

[nkuitse]

Kelly has the same take on CostDetails. Any chance of merging today or early next week?

[remocrevo]

I still want to make sure @PaulPoulain wasn't experiencing a different bug. If he can confirm that his data wasn't actually lost, then we can prepare to merge. I'll try to catch him on IRC on Monday.

[remocrevo]

@nkuitse I think we still need some user documentation for this branch. I see the upgrade instructions, but nothing about how to use the new features.

[benheet]

@nkuitse you might be able to use the original spec to update existing user guide

[nkuitse]

Where is the existing user guide? Our docs for 1.3 are here:

https://docs.google.com/document/d/1tFLgc55hXSoU7aQ-pHjm09JAQaO40cZcIQ64LuGFb7Q/edit?usp=sharing

I've generated a PDF of this but don't see where to put it.

[PaulPoulain]

OK, I tried to test again, and, good news, the acquisition type empty does not come from the patch. It came from my sample database, that had it empty. Sorry for the useless warning.

About the datepicker, I'm not sure I understand: applying only this patch, I still have the dates in US format. But I should ignore the problem, because it will be fixed by a jquery upgrade ? If yes, I'm fine with that, you can merge this patch imo.

[nkuitse]

I'm glad to hear that the acquisition types weren't deleted. As for the US-centric display of dates -- it's not right, but it seems that's the way dates have always been displayed in CORAL Resources so at least we're not making matters worse.

[remocrevo]

I have added a GitHub issue for improving the display of dates (issue #38). Otherwise, I believe this branch is ready to be merged into master. Thanks @nkuitse for the updated user guide. The PDF could be added to the CORAL website once we get login accounts figured out. @PaulPoulain are you able to work on that? Anything you need from us?

[PaulPoulain]

am I able to work on that => you mean date display ?
Yes, definetly, that's something I'll work on with @veggiematts and our intern @Cesarfr

[PaulPoulain]

(and maybe the jquery update will help us fixing this annoying problem)

[benheet]

Remington, do you want to go ahead and do the merge? We can post the updated guide to the discussion listserv until it has a permanent home on the website.

[benheet]

@nkuitse can you email me, bjheet@gmail.com, your PDF copy of the updated resources module user guide? We'll replace the existing one on the coral website with it.

@remocrevo would you mind handling the merge? I think we are ready.

@nkuitse you may want to start crafting an announcement and get ready to send it to coral-erm@listserv.nd.edu. I think its past time to get this one finished and out there.

[nkuitse]

I've just sent Ben the PDF. We'll work on an announcement. Thanks again!