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.

Srikanth, please obsolete the original patch when you attach your signed off patch - thank you.

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?

> 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!

Resetting complexity, because it touches C4, Koha and includes a database update.

(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

Hi Kyle, sorry, I should have given you the link:
http://wiki.koha-community.org/wiki/SubmitingAPatch

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

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.

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

Verified, it's <<today>> showing 21:6 - missing the 0.

(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.

Working on this.

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.

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.

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?

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?

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?

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.

(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

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

Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>

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.

This patch has been pushed to master. I corrected a typo (extra my) in the unit test while pushing.

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 );

(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.

I think we should not add the output of a time normal date fields. It's irritating to the users and not very helpful.

(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 :)

Cool, I'm glad - thx Jonathan! :)