Bug 9014 - Add time due to slips
Summary: Add time due to slips
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-06 19:11 UTC by Kyle M Hall
Modified: 2014-05-26 21:04 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9014 - Add time due to slips (5.07 KB, patch)
2012-11-06 19:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9014 - Add time due to slips (5.07 KB, patch)
2012-11-06 19:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 9014 - Add time due to slips (5.10 KB, patch)
2012-11-19 02:29 UTC, Srikanth Dhondi
Details | Diff | Splinter Review
Bug 9014 - Add time due to slips (5.07 KB, patch)
2012-12-02 20:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9014 - Add time due to slips (5.70 KB, patch)
2012-12-06 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9014 - Add time due to slips (5.74 KB, patch)
2012-12-06 14:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9014 - Add time due to slips (12.73 KB, patch)
2012-12-17 13:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9014 - Add time due to slips (13.07 KB, patch)
2013-01-02 21:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9014 - Add time due to slips (13.24 KB, patch)
2013-01-02 21:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9014 - Add time due to slips (13.28 KB, patch)
2013-01-03 15:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
patch rebased (13.30 KB, patch)
2013-03-19 15:35 UTC, Christophe Croullebois
Details | Diff | Splinter Review
Bug 9014 - Add time due to slips (13.35 KB, patch)
2013-03-19 16:01 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 9014 - Add time due to slips (13.54 KB, patch)
2013-03-19 16:27 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-11-06 19:11:24 UTC
We have a number of libraries that think that it would be helpful for patrons to
have the “time due” printed on the slip provided to a patron.
Comment 1 Kyle M Hall 2012-11-06 19:12:18 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-11-06 19:14:24 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-11-06 19:18:27 UTC
This patch modifies Koha::DateUtils::output_pref to support the new system preference TimeFormat, which defines the visual format for a time as either the 24 hour format ( default ), or the 12 hour format (HH:MM AM/PM).

TimeFormat only takes affect if dateformat is set to 'us'.

The patch also modifies C4::Members::IssueSlip to use output_pref rather than format_date.

TEST PLAN:
1) Apply patch
2) Run updatedatabase.pl
3) Issue an item to a patron, verify the times are in 24 hour format.
4) Switch TimeFormat to the 12 hour format.
5) Revist the patron record you issued an item to, times should now be in a 12 hour format.
6) Print a slip for this patron, you should now see the time as well as the date.
Comment 4 Srikanth Dhondi 2012-11-19 02:29:26 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2012-12-02 20:14:12 UTC
Srikanth, please obsolete the original patch when you attach your signed off patch - thank you.
Comment 6 Katrin Fischer 2012-12-02 20:44:04 UTC
I tested this and noticed a couple of things:

1) I noticed that 21:07 got formatted as 21:7 on the circulation slips.

2) The description for the new system preference has a typo: 
   'Format times sing'.

3) 12hour format only works with in combination with US date format. 
   I think it should be available for all date formats or the description 
   of the system preference should be changed.

4) When 12h pref and us date format are selected, the slips still print with 24h  
   for time for me. I tried shift reload and the due dates in the patron account 
   show correctly.

Kyle, could you check?
Comment 7 Katrin Fischer 2012-12-02 20:46:50 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2012-12-06 14:25:47 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-12-06 14:31:03 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-12-06 14:32:39 UTC
> 1) I noticed that 21:07 got formatted as 21:7 on the circulation slips.

I haven't been able to reproduce this problem.
 
> 2) The description for the new system preference has a typo: 
>    'Format times sing'.

Fixed
 
> 3) 12hour format only works with in combination with US date format. 
>    I think it should be available for all date formats or the description 
>    of the system preference should be changed.

Fixed

> 4) When 12h pref and us date format are selected, the slips still print with
> 24h  
>    for time for me. I tried shift reload and the due dates in the patron
> account 
>    show correctly.

I also haven't been able to reproduce this either.

Would you mind retesting this new patch to see if you still have problems 1 and 4?

Thanks!
Comment 11 Katrin Fischer 2012-12-06 14:46:24 UTC
Resetting complexity, because it touches C4, Koha and includes a database update.
Comment 12 Kyle M Hall 2012-12-06 15:21:01 UTC
(In reply to comment #11)
> Resetting complexity, because it touches C4, Koha and includes a database
> update.

Ok, are there any guidelines on the wiki for how to set the patch complexity?

Kyle
Comment 13 Katrin Fischer 2012-12-06 15:29:22 UTC
Hi Kyle, sorry, I should have given you the link:
http://wiki.koha-community.org/wiki/SubmitingAPatch
Comment 14 Kyle M Hall 2012-12-06 16:29:44 UTC
No problem! I was just unable to find it by searching the wiki. Thanks!

(In reply to comment #13)
> Hi Kyle, sorry, I should have given you the link:
> http://wiki.koha-community.org/wiki/SubmitingAPatch
Comment 15 Katrin Fischer 2012-12-16 22:00:41 UTC
Hi Kyle,

this is a lot better, but I found 2 issues:

1) The <<today>> placeholder in the notices module doesn't follow the new system preference and I think it's also where I saw the 21:7 showing up. Could you take a look at that? I think libraries using the 12 hour format will be the first to notice that the dates are formatted differently on the same slip.

2) The DateUtils.t test fails when you have TimeFormat set to 12hour time format. I notice the tests don't fail for different DateFormat settings. I think the tests should work for both time formats and it will save us some trouble if that could be fixed.
Comment 16 Katrin Fischer 2012-12-16 22:02:10 UTC
Test Summary Report
-------------------
t/DateUtils.t (Wstat: 2048 Tests: 28 Failed: 8)
  Failed tests:  5, 7, 9, 11-12, 25, 27-28
  Non-zero exit status: 8
Files=1, Tests=28,  1 wallclock secs ( 0.03 usr  0.00 sys +  0.24 cusr  0.01 csys =  0.28 CPU)
Result: FAIL
Comment 17 Katrin Fischer 2012-12-16 22:06:13 UTC
Verified, it's <<today>> showing 21:6 - missing the 0.
Comment 18 Kyle M Hall 2012-12-17 13:39:48 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2012-12-17 13:41:17 UTC
(In reply to comment #15)
> Hi Kyle,
> 
> this is a lot better, but I found 2 issues:
> 
> 1) The <<today>> placeholder in the notices module doesn't follow the new
> system preference and I think it's also where I saw the 21:7 showing up.
> Could you take a look at that? I think libraries using the 12 hour format
> will be the first to notice that the dates are formatted differently on the
> same slip.

Fixed!

> 2) The DateUtils.t test fails when you have TimeFormat set to 12hour time
> format. I notice the tests don't fail for different DateFormat settings. I
> think the tests should work for both time formats and it will save us some
> trouble if that could be fixed.

Fixed as well. I've added a couple new tests for 24hr vs 12hr time formats.
Comment 20 Katrin Fischer 2013-01-02 07:18:52 UTC
Working on this.
Comment 21 Katrin Fischer 2013-01-02 21:02:48 UTC
Hi Kyle,

2 problems with this:

1) The <<today>> is now off by 1 hour for me. We got 10 PM here, but the checkout slip says 9 PM. 

2) The test count was off, please remember to adjust the count when adding new tests. I fixed it and rebased the patch, attaching my updated patch here.
Comment 22 Katrin Fischer 2013-01-02 21:06:08 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2013-01-02 21:07:19 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2013-01-03 13:38:02 UTC
Could this possibly a configuration issue with your server? I cannot see anything in the code that could make the hour be off by 1.

(In reply to comment #21)
> 1) The <<today>> is now off by 1 hour for me. We got 10 PM here, but the
> checkout slip says 9 PM.
Comment 25 Katrin Fischer 2013-01-03 13:42:12 UTC
My installation runs directly on my Ubuntu powered laptop. The system time shown in the top right corner was different to the time on the slip. So I think that should not be the problem.
Could it be related timezones, daylight saving... whatever?
Comment 26 Kyle M Hall 2013-01-03 13:51:35 UTC
I would assume since it is an hour off that perhaps it is related to daylight savings. The timezone is set from $ENV{TZ} so I would assume it is using the system timezone. I'll retest my patch and see what results I get. 

(In reply to comment #25)
> My installation runs directly on my Ubuntu powered laptop. The system time
> shown in the top right corner was different to the time on the slip. So I
> think that should not be the problem.
> Could it be related timezones, daylight saving... whatever?
Comment 27 Kyle M Hall 2013-01-03 13:56:24 UTC
Everything looks right on my machine. Are both the times on circulation.pl and the time on the slip incorrect for you?

(In reply to comment #26)
> I would assume since it is an hour off that perhaps it is related to
> daylight savings. The timezone is set from $ENV{TZ} so I would assume it is
> using the system timezone. I'll retest my patch and see what results I get. 
> 
> (In reply to comment #25)
> > My installation runs directly on my Ubuntu powered laptop. The system time
> > shown in the top right corner was different to the time on the slip. So I
> > think that should not be the problem.
> > Could it be related timezones, daylight saving... whatever?
Comment 28 Katrin Fischer 2013-01-03 13:58:14 UTC
Only the time on the slip was wrong - as this shows the current time. All my items had due dates 23:59 - so not much to see there.
Comment 29 Kyle M Hall 2013-01-03 15:09:52 UTC
(In reply to comment #28)
> Only the time on the slip was wrong - as this shows the current time. All my
> items had due dates 23:59 - so not much to see there.

Here is a video of me testing. Could you check it out and let me know if I'm looking in the wrong place?

http://screencast.com/t/6KzLxP67h
Comment 30 Kyle M Hall 2013-01-03 15:35:06 UTC Comment hidden (obsolete)
Comment 31 cedric.vita 2013-03-18 13:09:04 UTC
Something went wrong !
Applying: Bug 9014 - Add time due to slips
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Letters.pm
CONFLICT (content): Merge conflict in C4/Letters.pm
Auto-merging C4/Members.pm
Auto-merging acqui/lateorders.pl
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/updatedatabase.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 9014 - Add time due to slips
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 9014 - Add time due to slips

Bug 9014 - Add time due to slips
Apply? [yn]
Patch left in /tmp/Bug-9014---Add-time-due-to-slips-Guckby.patch
Comment 32 Christophe Croullebois 2013-03-19 15:35:10 UTC Comment hidden (obsolete)
Comment 33 Paul Poulain 2013-03-19 16:01:18 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 34 Paul Poulain 2013-03-19 16:01:37 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2013-03-19 16:27:06 UTC
Created attachment 16424 [details] [review]
[PASSED QA] Bug 9014 - Add time due to slips

This patch modifies Koha::DateUtils::output_pref to support the new system preference TimeFormat, which defines the visual format for a time as either the 24 hour format ( default ), or the 12 hour format (HH:MM AM/PM).

The patch also modifies C4::Members::IssueSlip to use output_pref rather than format_date.

Test Plan:
1) Apply patch
2) Run updatedatabase.pl
3) Issue an item to a patron, verify the times are in 24 hour format.
4) Switch TimeFormat to the 12 hour format.
5) Revisit the patron record you issued an item to, times should now be in a 12 hour format.
6) Print a slip for this patron, you should now see the time as well as the date.

Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests after fixing the test count in t/DateUtils.t.
Fixed conflicts in syspref.sql and updatedatabase.pl.

Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
As we have another sign-off on this now I gave it a quick
run through and it works as expected.
All tests and QA script pass.
Comment 36 Jared Camins-Esakov 2013-03-21 15:09:07 UTC
This patch has been pushed to master. I corrected a typo (extra my) in the unit test while pushing.
Comment 37 Jonathan Druart 2013-05-30 10:19:44 UTC
This patch reorder the parameter for output_pref but does not deal with existing occurrence. I think there are 2 calls to fix:

C4/Letters.pm:        $values->{'expirationdate'} = output_pref( $dt, undef, 1 );
C4/Letters.pm:        $values->{'waitingdate'} = output_pref( dt_from_string( $values->{'waitingdate'} ), undef, 1 );
Comment 38 Jonathan Druart 2013-05-30 12:45:12 UTC
(In reply to comment #37)
> This patch reorder the parameter for output_pref but does not deal with
> existing occurrence. I think there are 2 calls to fix:
> 
> C4/Letters.pm:        $values->{'expirationdate'} = output_pref( $dt, undef,
> 1 );
> C4/Letters.pm:        $values->{'waitingdate'} = output_pref(
> dt_from_string( $values->{'waitingdate'} ), undef, 1 );

Not a big issue.
Dates reserves.expirationdate and reserves.waitingdate will be displayed like (depending from the dateformat pref):
  DD/MM/YYYY 00:00
instead of
  DD/MM/YYYY

I provided a patch (bug 10380) in order to fix the prototype problem of this routine.
Comment 39 Katrin Fischer 2013-05-30 13:27:32 UTC
I think we should not add the output of a time normal date fields. It's irritating to the users and not very helpful.
Comment 40 Jonathan Druart 2013-05-30 13:34:25 UTC
(In reply to comment #39)
> I think we should not add the output of a time normal date fields. It's
> irritating to the users and not very helpful.

My patch fixes that :)
Comment 41 Katrin Fischer 2013-05-30 13:36:57 UTC
Cool, I'm glad - thx Jonathan! :)