Bug 8296

Patch reformatted so it applies, I haven't tested well enough to sign off though, but this should help someone else

i test with a sandbox .it work.
In the opac with have the two dates shown(AAAA-MM-JJ and text) 


***

an automatic mode for managing the date formats through authorised values ​​activated in subscriptions records would also a idea ... but this does not mean the same developement.....

***

Why do you add a limitation of 15 chars in the tt file and 100 chars for the sql column ?

Hmm I suspect a copy/paste error...
Fix is coming.

Both patch rebased on master

QA Comments:

* serials/serials-collection.pl                                            FAIL
	forbidden patterns          FAIL
		The patch introduces a forbidden pattern: tabulation character at line 2 +	my $sth = $dbh->prepare(";
	valid                       OK
	critic                      OK
* serials/serials-edit.pl                                                  FAIL
	forbidden patterns          OK
	valid                       FAIL
		"my" variable @publisheddates masks earlier declaration in same scope 
	critic                      OK

1 tabulation character and 1 redefined variable.

Marked as Failed QA.

Please resolve merge conflict for serials-collection.pl on current master.
Thank you very much.

All patches rebased on master

QA Comment:

Add a new DB field serial.publisheddatetext.
The patch adds this new field to the serials update routines and to the serial templates (serials-collection, serials-edit, subscription-detail and opac-full-serial-issues).

Marked as Passed QA.

I would like to get a second QA team sign-off on these patches.

In fact there is no sign-off on these patchs. Switch back to "Needs Signoff"

Something went wrong !
Applying: Bug 8296: Add descriptive (text) published date field for serials
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Serials.pm
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt
Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-full-serial-issues.tt
Auto-merging misc/cronjobs/serialsUpdate.pl
Auto-merging serials/serials-collection.pl
Auto-merging serials/serials-edit.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 8296: Add descriptive (text) published date field for serials
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 8296 - Add descriptive (text) published date field for serials

Bug 8296: Add descriptive (text) published date field for serials
Apply? [yn]
Patch left in /tmp/Bug-8296-Add-descriptive-text-published-date-field-ZKRcFi.patch

ok for me the patch applies correctly, you can test

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

One warning, no complaints from qa-tools.
Argument "" isn't numeric in numeric lt (<) at /usr/share/koha/testclone/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt line 241.
Seems not to be introduced by your patches, though.

I actually miss a test plan for this report.
I have the impresssion that the opac change is only visible under specific circumstances (pref value and/or unimarc??)

This needs more information/documentation. Please add. Thanks.

See this in the rebase of kohastructure:

+  `itemnumber` text default NULL,

Should not be reintroduced..

Patches rebased on master
serials.itemnumber is no longer added by these patches

Test plan:
- Search for a subscription and go to serial collection page
- check some serials and click edit
- modify text under "published on (text)" column and save
- in opac, go to record detail, then click "subscriptions" tab » "More details" » "Full history" tab
- you should see the text you entered.

I have started testing this yesterday and after sleeping over it, there is something that worries me:

1) After update, the new column publisheddatetext is empty in the database for all existing issues, but when you receive new issues, the field is always filled with the publisheddate if you don't change it manually. My feeling is, that we should not fill the field, when there is no new information in it. 
Also, is it helpful if you have to delete the date first before you can type in "April 2013"?

In the templates that would work nicely as they already fall back to the publisheddate when the new field is empty:
[% newserialloo.publisheddatetext || newserialloo.publisheddate %]

2) If you are going to touch the patches - could you also add the bug number to the updatedatabase?

3) The table in serials-edit.pl is a little broken, when you are adding items for each issue. The colspan needs to be increased to 6 there now in the last row.

<td colspan="5">
      <a id="label[% serialslis.subscriptionid %]...">
             Click to add item</a>

Hi Katrin, thank you for the review.
This patch fixes 1) 2) and 3)

Applying: Bug 8296: Add descriptive (text) published date field for serials
Using index info to reconstruct a base tree...
M	C4/Serials.pm
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt
M	koha-tmpl/opac-tmpl/prog/en/modules/opac-full-serial-issues.tt
M	serials/serials-edit.pl
Falling back to patching base and 3-way merge...
Auto-merging serials/serials-edit.pl
Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-full-serial-issues.tt
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-full-serial-issues.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Serials.pm
Patch failed at 0001 Bug 8296: Add descriptive (text) published date field for serials
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

All patches rebased on master

Patch applied cleanly, go forth and signoff

Applying: Bug 8296: Add descriptive (text) published date field for serials
Using index info to reconstruct a base tree...
M	C4/Serials.pm
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
M	misc/cronjobs/serialsUpdate.pl
Falling back to patching base and 3-way merge...
Auto-merging misc/cronjobs/serialsUpdate.pl
CONFLICT (content): Merge conflict in misc/cronjobs/serialsUpdate.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Serials.pm
Patch failed at 0001 Bug 8296: Add descriptive (text) published date field for serials
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

h1>Something went wrong !</h1>Applying: Bug 8296: Add descriptive (text) published date field for serials Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Serials.pm
CONFLICT (content): Merge conflict in C4/Serials.pm Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt
Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-full-serial-issues.tt
Auto-merging misc/cronjobs/serialsUpdate.pl Auto-merging serials/serials-collection.pl Failed to merge in the changes.
Patch failed at 0001 Bug 8296: Add descriptive (text) published date field for serials When you have resolved this problem run git bz apply --continue.

I add a text formatted date in the Serial collection page like this :

Date published 	Date published (text) 	
01/04/2014 	mai 2014 

but the date at the opac (moredetails/fullhistory/date column) remains 01/04/2014.

Olivier Crouzet

I've applied the patches, ran the database update and then tried to run the unit tests specific for serials, sadly, they are failing with these patches, but are ok on master:

Test Summary Report
-------------------
t/db_dependent/Serials.t (Wstat: 1024 Tests: 44 Failed: 4)
  Failed tests:  39-40, 42-43
  Non-zero exit status: 4


The QA script was ok, no problems there.

I am sorry, but I am having problems to get this to apply :( 
I fixed a conflict in the first patch but then got stuck later on number 5.

Hi Julian,

I have a few questions about this:

For the OPAC, you only change the full history:
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-full-serial-issues.tt 

Why not the normal one on opac-detail.tt?

If we only change one view, the displayed dates seem to be inconsistent.


I am also wondering about you display the publisheddate in staff, when there is nothing set for publisheddate. This way it's really hard to see in the lists, where you have entered custom data and where not.

I am suggesting to change that and only display publisheddatetext when we have both columns next to each other. 

I will attach a patch with that suggestion - but not sure what to do about the OPAC.

Ah, also applying rebaesd patches - I removed the prog theme OPAC files.

After 'sleeping over it' I think the duplication of the 'published date' into 'published date (text)' was maybe done for sorting purposes?

Maybe we could make it visually more clear, using a different text color (grey?) for a published date in a published date (text) column?

I hope this makes any sense. Feel free to dislike and obsolete my follow-up :)

Why is the bug status "Failed QA" ? Shouldn't it be "Needs Signoff" ?

Going to work on this next. Julian - could you check the last patch if you like or hate it? I have no problem with either, but would be good to have your feedback.

Hi Katrin,

Your patch is ok, no problem at all, it is already signed off ;)

Hi Julian, 

I have started on this and so far it looks good, but I am a little confused about the general idea of this:

- Could you maybe give some more explanation about the use case?
- Why do we display it almost everywhere in staff, but only on a very hidden page in the OPAC?

1/ The initial need was to "describe" the date in a more human-friendly format.
For instance, if periodicity is "once every two months", instead of 2015-01-01 we can have "2015 January-February"

2/ I don't know very well the subscriptions part in OPAC. I can add this information in other pages as well, if you tell me where :)

Hi Julian, 

I think the common "entry page" to subscriptions for parons in the OPAC is opac-detail > subscriptions tab. This view is labelled as 'Normal view' when you look at it from the the full history page. I think adding the information there the same way we have it on the full history page would make sense to me. 

I am a bit torn about the feature still, but I think with that follow-up it would make enough sense to me to pass QA. Patches tested ok so far. :)

(In reply to Katrin Fischer from comment #103)
> Hi Julian, 
> 
> I think the common "entry page" to subscriptions for parons in the OPAC is
> opac-detail > subscriptions tab. This view is labelled as 'Normal view' when
> you look at it from the the full history page. I think adding the
> information there the same way we have it on the full history page would
> make sense to me. 

It appears that the date shown on this page is the 'planneddate' which is the date of reception. Should we change that and display the 'publisheddate' instead ?

(In reply to Julian Maurice from comment #111)
> (In reply to Katrin Fischer from comment #103)
> > Hi Julian, 
> > 
> > I think the common "entry page" to subscriptions for parons in the OPAC is
> > opac-detail > subscriptions tab. This view is labelled as 'Normal view' when
> > you look at it from the the full history page. I think adding the
> > information there the same way we have it on the full history page would
> > make sense to me. 
> 
> It appears that the date shown on this page is the 'planneddate' which is
> the date of reception. Should we change that and display the 'publisheddate'
> instead ?

Sorry to come back to this so late - I feel like it is a little weird to not display in the OPAC, but not sure right now how best to fix. I will look at the patches again.

Created attachment 43665 [details] [review]
[PASSED QA] Bug 8296: Add descriptive (text) published date field for serials

Add a new field serial.publisheddatetext for displaying published date
in another format that dateformat syspref permit.

Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 43666 [details] [review]
[PASSED QA] Bug 8296: set input maxlength to the same of sql field length

Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 43667 [details] [review]
[PASSED QA] Bug 8296: Follow up for QA

1 redefined variable removed

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 43668 [details] [review]
[PASSED QA] Bug 8296: Don't fill text field automatically

+ minor fixes:
  * Add bug number to updatedatabase
  * Fix colspan in serials-edit.tt

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 43669 [details] [review]
[PASSED QA] Bug 8296: Display publisheddatetext in OPAC bootstrap theme

and remove modifications for OPAC prog theme

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 43670 [details] [review]
[PASSED QA] Bug 8296: Fix unit tests in t/db_dependent/Serials.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 43671 [details] [review]
[PASSED QA] Bug 8296: Follow-up - don't display publisheddate when publisheddatetext is empty

To test:
- make sure you have a subscription where only some fields have a published date (text) set
- check the subscription detail page, issues tab
- check the serial collection page
- both pages should show an empty field, when published date (text) has not been set

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

I have to admit, I am not sure about the use case for this patch, but it works as described and shouldn't affect the existing workflows.

Patches pushed to master.

Thanks Julian!

Created attachment 45469 [details]
serials area where text shows

I'm documenting this and am wondering where the text displays other than in the serials module. Shouldn't is show in the holdings area?

Hi Nicole,

3 spots it shows are:

Staff
- subscription detail page, issues tab
- serial collection page

OPAC
- full history

There was some discussion about where else it should show - so there is probably some room for improvement :)