Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pr4iss15.17: Add enhanced cost history new fields to export and print view #43

Merged
merged 9 commits into from
Sep 16, 2016

Conversation

fondrenlibrary
Copy link
Contributor

@fondrenlibrary fondrenlibrary commented May 20, 2016

Used to solve issue #15 and #17 plus add basic date validity check to the date fields in cost history.

@jeffnm jeffnm added enhancement This is an enhancement (not a bug) needs testing labels Jun 6, 2016
@jeffnm jeffnm added this to the Version 2.0.0 Beta milestone Jun 6, 2016
@fondrenlibrary
Copy link
Contributor Author

fondrenlibrary commented Jun 6, 2016

Will wait for Sirsi' s upcoming merge which might affect the fund code field and then make some corresponding updates to this PR if necessary. Also waiting for PR #3 to be committed first.

@remocrevo remocrevo changed the title Pr4iss15.17 Pr4iss15.17: Add enhanced cost history new fields to export and print view Jul 11, 2016
@@ -283,3 +282,40 @@ function postwith (to,p) {
document.body.removeChild(myForm) ;
}


function isValidDate(dateString)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of writing our own date validation code, we should consider using jQuery. Note: CORAL currently uses jQuery 1.4.4. Here is an example:
http://stackoverflow.com/questions/6598386/jquery-validation-date

Copy link
Contributor Author

@fondrenlibrary fondrenlibrary Aug 11, 2016

Choose a reason for hiding this comment

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

Yes, it's really handy and better to use JQuery/Plugin, however at the time when this PR was being written, the version of jQuery was found really behind. Now, after pulling all latest commits into this PR, I decide to completely give up date validation because Subscription dates still look not required and the underlying mySQL can automatically reset all invalid date inputs to 01/01/1970 after they are are submitted and later they can be changed at any time to good ones.

Copy link
Contributor Author

@fondrenlibrary fondrenlibrary Aug 11, 2016

Choose a reason for hiding this comment

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

After sync'd with the master branch and conflict resolving done, this PR can be tested again and ready to merge (before or after #63) . The following changes have been made to the original PR.

  1. Date validation dropped.
  2. Extra cost fields in the Cost column in "exported" CSV dropped. If later better idea can be acquired about how to present all cost fields in the Cost column, a new PR can be created.
  3. An issue with submission of Cost Form by costForm.js fixed. This issue has been introduced since a major commit in June

Copy link
Contributor

Choose a reason for hiding this comment

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

@remocrevo

Note: CORAL currently uses jQuery 1.4.4.

Actually Coral uses 4 different versions of jQuery so we have a broad choice ! ^^"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@remocrevo , nowadays it seems hard to find a working validation plugin for jquery 1.4.4 which is loaded by the Resource pages. On the other hand, the newest validation plugin can't handle leap years well as opposed to the validateDate() function added by this PR a while ago, though this is not a game changer at all. Maybe we also need to discuss jQuery versions in the upcoming SC meeting. For now, as mentioned in my early comment, I completely drop out the date validation function and we can re-add it later on separately. By the way, would you mind testing and merging this PR before this PR becomes conflicting again? Any direct editing and committing upon problems found is welcome. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, would you mind testing and merging this PR before this PR becomes conflicting again?

Is there a test plan (instructions to setup and test) somewhere? So anyone could test it even without having followed this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tuxayo , actually, no particular test plan nor any integration test plan. The only test by hand plan is set enhancedCostHistory=Y and watch whether all the enhanced fields can show up in Edit Cost Information and then watch whether Cost information can be saved. Also we need to observe whether those enhanced fields show in Print View which is under the Helpful Links. An extra benefit but not planned is this PR has fixed the error encountered upon saving Cost history. @tuxayo if your test proves fine to you, simply merge it. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fondrenlibrary

actually, no particular test plan nor any integration test plan. The only test by hand plan is [...]

Maybe test plan weren't the right words but yes I meant manual instructions to be able to check that this PR works as intended without prior knowledge. So I (or others) could manually test it and contribute to build enough confidence so it could be merged quicker.

Thanks, the instructions that you provided are exactly what I needed. I hope to have the time to try them.

@fondrenlibrary fondrenlibrary mentioned this pull request Aug 11, 2016
… text fields used to accept dates about Resource
…ues can be correctly captured and saved after costForm.js is modified to only allow selecting tax fields in .paymentTable TR (table row)
@fondrenlibrary
Copy link
Contributor Author

SC approved is assigned and ready to be merged (next week).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement (not a bug) SC approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants