Bug 14922 - Remove C4::Dates from cataloguing/additem.pl
Summary: Remove C4::Dates from cataloguing/additem.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marc Véron
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14870
  Show dependency treegraph
 
Reported: 2015-09-29 13:39 UTC by Marc Véron
Modified: 2017-06-14 22:10 UTC (History)
5 users (show)

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


Attachments
Bug 14922 - Remove C4::Dates from cataloguing/additem.pl (877 bytes, patch)
2015-09-29 13:44 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14922 - Remove C4::Dates from cataloguing/additem.pl (4.28 KB, patch)
2015-09-30 13:10 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 14922 - Remove C4::Dates from cataloguing/additem.pl (4.54 KB, patch)
2015-09-30 23:09 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 14922 - Remove C4::Dates from cataloguing/additem.pl (4.74 KB, patch)
2015-10-02 14:28 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14922 - (followup) Fix string output (1.34 KB, patch)
2015-10-07 18:50 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED OFF]Bug 14922 - Remove C4::Dates from cataloguing/additem.pl (4.75 KB, patch)
2015-10-07 20:06 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 14922 - (followup) Fix string output (1.43 KB, patch)
2015-10-07 20:06 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 14922 - Remove C4::Dates from cataloguing/additem.pl (4.81 KB, patch)
2015-10-15 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14922 - (followup) Fix string output (1.48 KB, patch)
2015-10-15 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2015-09-29 13:39:39 UTC
This patch removes C4::Dates from cataloguing/additem.pl
Comment 1 Marc Véron 2015-09-29 13:44:27 UTC Comment hidden (obsolete)
Comment 2 Héctor Eduardo Castro Avalos 2015-09-29 20:18:58 UTC
Hi, Marc.

The module C4::Dates is still used in line 504 
my $today_iso = C4::Dates->today('iso');
The scalar variable it's repeating in many places.

So I change here the bug status to in discussion

Regards
Comment 3 Marc Véron 2015-09-30 11:51:13 UTC
Comment on attachment 42951 [details] [review]
Bug 14922 - Remove C4::Dates from cataloguing/additem.pl

New patch will follow.
Comment 4 Marc Véron 2015-09-30 13:10:57 UTC Comment hidden (obsolete)
Comment 5 Héctor Eduardo Castro Avalos 2015-09-30 23:09:11 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-10-01 08:26:31 UTC
Marc,
What about using Koha::DateUtils to replace ->today?
Something like
 my $today = dt_from_string;
 my $month = $today->month;
 my $day = $today->day;
do what you want.
Comment 7 Marc Véron 2015-10-02 14:28:19 UTC Comment hidden (obsolete)
Comment 8 Héctor Eduardo Castro Avalos 2015-10-07 17:38:55 UTC
(In reply to Marc Véron from comment #7)
> Created attachment 43069 [details] [review] [review]
> Bug 14922 - Remove C4::Dates from cataloguing/additem.pl
> ... 
> Patch amended following comment #6  / mv

Hi, MARC I tested this patch with this kind of patterns:
Pattern: Year:YYYY Month:MM Day:DD
the result is:
Year:2015-10-07T11:26:57->year Mount:2015-10-07T11:26:57->month Date:2015-10-07T11:26:57->day

Pattern: YYYYMMDD
Result: 2015-10-07T11:35:32->year2015-10-07T11:35:32->month2015-10-07T11:35:32->day

I've been tested other sort of patterns but the result is the same.
Patch 14936 not applied since patch in Discussion and not mentioned in this bug.

I don't know if this behaviour in dates is right.

Regards
Comment 9 Marc Véron 2015-10-07 18:41:48 UTC
Hi Hector,
Thanks for testing - and silly me, I tested be editing an item where the value had already been set by previous tests.
Patch follows.
Comment 10 Marc Véron 2015-10-07 18:50:21 UTC Comment hidden (obsolete)
Comment 11 Héctor Eduardo Castro Avalos 2015-10-07 19:57:21 UTC
I was looking that there no info about this in documentation <http://manual.koha-community.org/3.14/en/catadmin.html#editsubfields>, section Advanced constraints or I don't know if this exist in another page.

I added here Documentation Manager.
Comment 12 Héctor Eduardo Castro Avalos 2015-10-07 20:06:15 UTC Comment hidden (obsolete)
Comment 13 Héctor Eduardo Castro Avalos 2015-10-07 20:06:26 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-10-08 08:44:31 UTC
Marc, I would prefer the first version, could be done with:

  my ( $yyyy, $mm, $dd ) = ( $today->year, $today->month, $today->day );
  $value =~ s/YYYY/$yyyy/g;
  $value =~ s/MM/$mm/g;
  $value =~ s/DD/$dd/g;

and avoid 1 call to output_string.
Especially because it's how we did on bug 14950 for batchMod.pl (also the default values).

But maybe we have an issue, the $today->day returns "8" instead of "08".
I don't know what is the expected behavior.
Comment 15 Marc Véron 2015-10-08 09:05:31 UTC
Hi Jonathan,

Would that be the same as the patch from comment #5, the first one signed-off by Héctor?
http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42987
Comment 16 Jonathan Druart 2015-10-08 10:54:54 UTC
I would prefer not to depend on something else than Koha::DateUtils.
Comment 17 Jonathan Druart 2015-10-15 09:24:34 UTC
Marc, I think that 14950 has introduced a bug (3 instead of 03), do you agree with that?
Comment 18 Marc Véron 2015-10-15 10:29:50 UTC
(In reply to Jonathan Druart from comment #17)
> Marc, I think that 14950 has introduced a bug (3 instead of 03), do you
> agree with that?

Do you mean because of tools/batchMod.pl near line 337?

my $month = $today->month;
my $day   = $today->day;

Output without leading 0  (3 instead of 03)?
Comment 19 Jonathan Druart 2015-10-15 10:44:28 UTC
yes, exactly, both should have the same behavior.
Comment 20 Jonathan Druart 2015-10-15 10:44:49 UTC
(and yes I know, it was my idea... sorry!)
Comment 21 Marc Véron 2015-10-15 11:11:26 UTC
(In reply to Jonathan Druart from comment #20)
> (and yes I know, it was my idea... sorry!)

No problem  :-)

Is it necessay to fix 14950 immediately? My problem is that I am now out of office for about 2 days...
Comment 22 Jonathan Druart 2015-10-15 12:57:12 UTC
Created attachment 43435 [details] [review]
Bug 14922 - Remove C4::Dates from cataloguing/additem.pl

This patch removes C4::Dates from cataloguing/additem.pl

To test:

- In MARC bibliographic framework, define a default value
  for one of the in Tag 952 Subfield struture, e.g. for
  subfield x Non-public note. You can define it under
  'Advancde constraints'. Define the value similar to the following:
  Year:YYYY Month:MM Day:DD
- Create a new item for a biblio and verify that YYYY, MM and DD are
  replaced with today's values.
- Apply patch and repeat that previous step behaves as before.
- Verify that the overall functionality of the form is the same
  as before applying the patch.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as advertised. With the last script not accept more than YYYYMMDD
If you enter something else, the script sends to the end of line.
This patch follow up the pattern in constraint default value

Patch amended following comment #6  / mv

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2015-10-15 12:57:18 UTC
Created attachment 43436 [details] [review]
Bug 14922 - (followup) Fix string output

This patch makes YYYY MM and DD display correctly.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as advertised

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2015-10-15 12:57:39 UTC
(In reply to Marc Véron from comment #21)
> (In reply to Jonathan Druart from comment #20)
> > (and yes I know, it was my idea... sorry!)
> 
> No problem  :-)
> 
> Is it necessay to fix 14950 immediately? My problem is that I am now out of
> office for about 2 days...

I have submitted a patch on bug 14950 to fix the issue.
Comment 25 Tomás Cohen Arazi 2015-10-19 14:54:20 UTC
Patches pushed to master.

Thanks Marc!
Comment 26 Nicole C. Engard 2015-11-30 21:41:23 UTC
(In reply to Héctor Eduardo Castro Avalos from comment #11)
> I was looking that there no info about this in documentation
> <http://manual.koha-community.org/3.14/en/catadmin.html#editsubfields>,
> section Advanced constraints or I don't know if this exist in another page.
> 
> I added here Documentation Manager.

I removed it from the manual when I found that it no longer worked. I'll add it in to the recent manual.

Nicole