The current irregularity feature works with saving internal fictive issue numbers in a text field irregularity. If you edited/shifted some dates in serial collection, you might be surprised what happens.
You cannot view the saved irregularities in serial collection (I only see e.g. 2 issues on the Planning tab); they are only visible when editing the subscription and looking at the prediction pattern again.

Bug 18365 and friends made it easier to handle discrepancies in the schedule. An occasional irregularity could be resolved that way.

Would it be useful to define recurring irregularities in terms of the serial frequency chosen? If it is month, we could allow the user to select one or more months to skip? Same for weeks or days. (Year seems less interesting.)
Doing this would replace selection of irregulars in the prediction pattern.

If so, how should we handle 'special' frequencies like 1 per 3 months or 3 per month btw? Skip all in e.g. July or refine with skip second in July etc.?

It seems to me that we should make the irregularity info more visible to the user too on Serial collection form.

Any feedback is appreciated.

I feel my expectation for this was always that it will be recurring. So say I have a serial prediction pattern for 1/month and I say there is never an issue in August (vacation?), I'd want this to stick. As I understood the feature, it would allow to have patterns that are not 'regular' in some odd ways.

Like 50 issues a year, mostly weekly, where the time between issues is not always the same, but some are just skipped.

Not sure if we could reach out to serial librarians on the mailing list maybe?

(In reply to Katrin Fischer from comment #2)
> Not sure if we could reach out to serial librarians on the mailing list
> maybe?

Sent to general ml.

We are looking for feedback on this one - adding some names to cc in hope of more comments!

Thread: https://lists.katipo.co.nz/pipermail/koha/2017-June/048380.html

Sadly no comments so far. So it's Lyon 3 and me so far considering this a bug, maybe enough?

(In reply to Katrin Fischer from comment #5)
> Thread: https://lists.katipo.co.nz/pipermail/koha/2017-June/048380.html
> 
> Sadly no comments so far. So it's Lyon 3 and me so far considering this a
> bug, maybe enough?

We can wait a little bit more :) Good things take time..
Hope to address this later, having some more feedback.

Hello Marcel,
I think it's a pity not to try to find a solution if we are several libraries that miss this feature.
Furthermore, it existed in the previous versions of Koha and for us, it was clearly  a regression when it disappeared.
If the solution proposed isn't correct, it will be Failed QA....

If you don't mind, I will change this bug title to the former one to give a chance to this feature to be back in koha.

Sonia

Hi Sonia,
it's good information that this is a regression to the older serials module. I agree that we need to bring it back.

Here is a proposal for keeping irregularities on subscription after renewal. This patch save days of week or month in a new column (permanent_irregularity). So it works only for daily and monthly subscriptions. I think that it doesn't make sense to do this for weekly... you opinion?

Hi Alex

When checking if Wednesday and Thursday are still checked I have found that the test prediction pattern div is no longer being displayed.

The errors I am getting in my intranet-error.log are:

0.0.2.2:34032] AH01215: [Fri Oct 20 00:20:18 2017] showpredictionpattern.pl: Argument "month" isn't numeric in numeric eq (==) at /home/vagrant/kohaclone/serials/showpredictionpattern.pl line 116., referer: http://localhost:8081/cgi-bin/koha/serials/subscription-add.pl?op=modify&subscriptionid=4
[Fri Oct 20 00:20:18.672718 2017] [cgi:error] [pid 7358] [client 10.0.2.2:34032] AH01215: [Fri Oct 20 00:20:18 2017] showpredictionpattern.pl: Argument "day" isn't numeric in numeric eq (==) at /home/vagrant/kohaclone/serials/showpredictionpattern.pl line 116., referer: http://localhost:8081/cgi-bin/koha/serials/subscription-add.pl?op=modify&subscriptionid=4
[Fri Oct 20 00:20:18.678244 2017] [cgi:error] [pid 7358] [client 10.0.2.2:34032] AH01215: [Fri Oct 20 00:20:18 2017] showpredictionpattern.pl: The method permanent_irregularity is not covered by tests!, referer: http://localhost:8081/cgi-bin/koha/serials/subscription-add.pl?op=modify&subscriptionid=4

Your t/db_dependent/Serials/SubscriptionIrregularity.pm test also fails because there is no test for permanent_irregularity function

Hello Alex,

Good thing for dayly and monthly publication.
As for weekly issues, don't know if it's realistic but what about basing calculation on week numbering (iso 8601) ?
Some review (not so unusual cases) announce clearly their "not issued weeks". Example the New Yorker publishes 5 issues that runs along 2 weeks and they are cited in the publication information block.

Olivier Crouzet

(In reply to Alex Buckley from comment #12)
> Hi Alex
> 
> When checking if Wednesday and Thursday are still checked I have found that
> the test prediction pattern div is no longer being displayed.
> 
> The errors I am getting in my intranet-error.log are:
> 
> 0.0.2.2:34032] AH01215: [Fri Oct 20 00:20:18 2017] showpredictionpattern.pl:
> Argument "month" isn't numeric in numeric eq (==) at
> /home/vagrant/kohaclone/serials/showpredictionpattern.pl line 116., referer:
> http://localhost:8081/cgi-bin/koha/serials/subscription-add.
> pl?op=modify&subscriptionid=4
> [Fri Oct 20 00:20:18.672718 2017] [cgi:error] [pid 7358] [client
> 10.0.2.2:34032] AH01215: [Fri Oct 20 00:20:18 2017]
> showpredictionpattern.pl: Argument "day" isn't numeric in numeric eq (==) at
> /home/vagrant/kohaclone/serials/showpredictionpattern.pl line 116., referer:
> http://localhost:8081/cgi-bin/koha/serials/subscription-add.
> pl?op=modify&subscriptionid=4
> [Fri Oct 20 00:20:18.678244 2017] [cgi:error] [pid 7358] [client
> 10.0.2.2:34032] AH01215: [Fri Oct 20 00:20:18 2017]
> showpredictionpattern.pl: The method permanent_irregularity is not covered
> by tests!, referer:
> http://localhost:8081/cgi-bin/koha/serials/subscription-add.
> pl?op=modify&subscriptionid=4
I will fix the warning "Argument x isn't numeric" while doing the patch for weekly.

> 
> Your t/db_dependent/Serials/SubscriptionIrregularity.pm test also fails
> because there is no test for permanent_irregularity function
You must Launch misc/devel/update_dbix_class_files.pl to update Koha/Schema/Result/Subscription.pm

(In reply to Koha Team Lyon 3 from comment #13)
> Hello Alex,
> 
> Good thing for dayly and monthly publication.
> As for weekly issues, don't know if it's realistic but what about basing
> calculation on week numbering (iso 8601) ?
> Some review (not so unusual cases) announce clearly their "not issued
> weeks". Example the New Yorker publishes 5 issues that runs along 2 weeks
> and they are cited in the publication information block.
> 
> Olivier Crouzet

Hello Olivier,

I did it for weekly serials. Please test and give me your opinion.

Created attachment 68665 [details] [review]
Bug 17656 - Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for monthly, daily or weekly serials ( 1 unit per issue
only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Ok Alex, I will do it as soon as possible

Test plan works as described, but QA tools warns of forbidden patterns.

koha-tmpl/intranet-tmpl/prog/en/includes/serials/subscription_daily_choice.inc
   FAIL	  forbidden patterns
		forbidden pattern: Do not use line breaks inside template tags (bug 18675) (line 5)

(In reply to David Bourgault from comment #20)
> Test plan works as described, but QA tools warns of forbidden patterns.
> 
> koha-tmpl/intranet-tmpl/prog/en/includes/serials/subscription_daily_choice.
> inc
>    FAIL	  forbidden patterns
> 		forbidden pattern: Do not use line breaks inside template tags (bug 18675)
> (line 5)

Hello David,

I have no warning when launching the qa script (koha-qa.pl -v 3 -c 1)
For the file subscription_daily_choice file i got:

 OK     koha-tmpl/intranet-tmpl/prog/en/includes/serials/subscription_daily_choice.inc
   OK     forbidden patterns
   OK     git manipulation
   OK     spelling
   OK     tt_valid
   OK     valid_template

(In reply to Alex Arnaud from comment #21)
> (In reply to David Bourgault from comment #20)
> > Test plan works as described, but QA tools warns of forbidden patterns.
> > 
> > koha-tmpl/intranet-tmpl/prog/en/includes/serials/subscription_daily_choice.
> > inc
> >    FAIL	  forbidden patterns
> > 		forbidden pattern: Do not use line breaks inside template tags (bug 18675)
> > (line 5)
> 
> Hello David,
> 
> I have no warning when launching the qa script (koha-qa.pl -v 3 -c 1)
> For the file subscription_daily_choice file i got:
> 
>  OK    
> koha-tmpl/intranet-tmpl/prog/en/includes/serials/subscription_daily_choice.
> inc
>    OK     forbidden patterns
>    OK     git manipulation
>    OK     spelling
>    OK     tt_valid
>    OK     valid_template

Sorry you are right. My QA tools was not up to date

Created attachment 69124 [details] [review]
Bug 17656 - Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for monthly, daily or weekly serials ( 1 unit per issue
only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Created attachment 69207 [details] [review]
Bug 17656 - Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for monthly, daily or weekly serials ( 1 unit per issue
only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Test plan worked.

QA tool all green except for C4/Serials.pm which warned that "POD coverage was greater before". Inspecting the file manually, this doesnt seem to be the case. 
I suspect that it is triggered because you remove a function and its pod, which it shouldn't warn about.

Signed off.

Hello Alex,
For the three types of periodicity, the "permanent" irregularities on top of table are kept over the renewal. This is working. But the detail of excluded issues is not automatically set in relation with these ones (cf joined image).We have to manually uncheck and recheck the "permanent" irregularities to have the corresponding issues checked in the "Not published column".

That's a progress in the sense that there's a trace of the previous configuration but the idea was to get all this automatically. would it be possible ?

Olivier Crouzet

Created attachment 69279 [details]
monthly irregularities test example

Hello Olivier,

Please re-try with this patch (just a typo in the previous).

Bonjour Alex
It's ok now as far as keeping irregularities is concerned : the corrresponding issues  to exclude are now automatically checked when checking the top table of "permanent" irregularities.
But there's one problem remaining about multi-receiving weekly serials.
The excluded issues are correctly skipped when receiving issues one by one but not when receiving through multi-receiving function.

For the others two periodicities (daily and monthly), it also works with multi-receiving.

I tested without patch and the problem disappears.

Olivier Crouzet

Olivier, thx for your great testing!

(In reply to Koha Team Lyon 3 from comment #30)
> Bonjour Alex
> It's ok now as far as keeping irregularities is concerned : the
> corrresponding issues  to exclude are now automatically checked when
> checking the top table of "permanent" irregularities.
> But there's one problem remaining about multi-receiving weekly serials.
> The excluded issues are correctly skipped when receiving issues one by one
> but not when receiving through multi-receiving function.
> 
> For the others two periodicities (daily and monthly), it also works with
> multi-receiving.
> 
> I tested without patch and the problem disappears.
> 
> Olivier Crouzet

Hello Olivier,

Can you write a test plan for this bug? I've tried the following one and it works for:

  - create a weekly serial from week #44 to week 50 (7 issues),
  - place irregularities on week #47 and #48, save,
  - click on Receive button on subscription detail page,
  - click on multi-receiving,
  - How many issues do you want to receive ? => 7
  - I got 5 issues generated. #47 and #48 has been skipped

Hello Alex
I tested again and realized that the problem is not bound to multi-receiving function. I was focusing last time on verifying at each renewal wether issues checkboxes was checked or not and I almost missed it (and my checkings were followed by a subscription saving as a reflex).
So the remaining problem is that the irregularities are not fit to new subscription unless you edit the subscription, run the test and save.
If you just make the renewal the irregularities(field irregularity) are not updated.

When testing I fall again upon a bug that I have already posted for ( 	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15484). It's also tight with irregularities and it would be great to have look at it.

Olivier

Created attachment 69820 [details] [review]
Bug 17656 - Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for monthly, daily or weekly serials ( 1 unit per issue
only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Created attachment 69915 [details] [review]
Bug 17656 - Update irregularities on subscription renewals

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Renew subscription from 2017-10-16
  - Check that you have the prediction pattern table on renwal page
  - Submit renewal
  - Check that irregularity and permanent_irregularity column
    have been updated.

  - Do the same for a monthly and weekly subscription

Hello Olivier,

The last patch add the prediction parttern table to the renewal pop-up. While renewing, you can change irregularities or let them as they were before. I think this is better than automatically update irregularities without asking the user.

Please test

Alex

Hey Alex, Happy new year
I tested again the three periodicity and it works like a charm ! I agree with you about not renewing automatically and being able to modify irregularities. The more flexibility, the better.

Just a thing about why I don't sign yet : I tested on top master and I had to solve a conflict (in the first patch : staff-global.css file) to be able to apply the patch.
I leave you rebase the commit and I will sign it right away.

Created attachment 70335 [details] [review]
Bug 17656 - Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for monthly, daily or weekly serials ( 1 unit per issue
only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Created attachment 70336 [details] [review]
Bug 17656 - Update irregularities on subscription renewals

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Renew subscription from 2017-10-16
  - Check that you have the prediction pattern table on renwal page
  - Submit renewal
  - Check that irregularity and permanent_irregularity column
    have been updated.

  - Do the same for a monthly and weekly subscription

(In reply to Koha Team Lyon 3 from comment #38)
> 
> Just a thing about why I don't sign yet : I tested on top master and I had
> to solve a conflict (in the first patch : staff-global.css file) to be able
> to apply the patch.
> I leave you rebase the commit and I will sign it right away.

Hello Olivier, patches rebased. Happy new year!

it works, thank you Alex !

Comment on attachment 70335 [details] [review]
Bug 17656 - Keep irregularities during subscription renewals

Review of attachment 70335 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/includes/serials/subscription_daily_choice.inc
@@ +1,5 @@
> +<p><em>
> +    If there is a day (or more) in the week where issues are never
> +    published, you can check corresponding boxes below.
> +</em></p>
> +[% daily_choice = [ {name => 'monday', desc => 'Monday', value => 1}, {name => 'tuesday', desc => 'Tuesday', value => 2}, {name => 'wednesday', desc => 'Wednesday', value => 3}, {name => 'thursday', desc => 'Thursday', value => 4}, {name => 'friday', desc => 'Friday', value => 5}, {name => 'saturday', desc => 'Saturday', value => 6}, {name => 'sunday', desc => 'Sunday', value => 7} ] %]

This way the day names are not translatable

::: koha-tmpl/intranet-tmpl/prog/en/includes/serials/subscription_monthly_choice.inc
@@ +1,5 @@
> +<p><em>
> +    If there is a month (or more) in the year where issues are never
> +    published, you can check corresponding boxes below.
> +</em></p>
> +[% monthly_choice = [ {name => 'january', desc => 'January', value => 1}, {name => 'february', desc => 'February', value => 2}, {name => 'march', desc => 'March', value => 3}, {name => 'april', desc => 'April', value => 4}, {name => 'may', desc => 'May', value => 5}, {name => 'june', desc => 'June', value => 6}, {name => 'july', desc => 'July', value => 7}, {name => 'august', desc => 'August', value => 8}, {name => 'september', desc => 'September', value => 9}, {name => 'october', desc => 'October', value => 10}, {name => 'november', desc => 'November', value => 11}, {name => 'december', desc => 'December', value =>12} ] %]

Month names are not translatable

::: t/Koha/DateUtils.t
@@ +26,5 @@
> +
> +my $dt = DateTime->new(year => 2017, month => 1, day => 1);
> +
> +eval { issue_number('2017', 'day'); };
> +is($@, 'Date passed to issue_number is not a valid DateTime object');

Could you please also test if the ref() of exception has right class?

@@ +40,5 @@
> +is(issue_number($dt, 'day'), 6, '2022-09-17 is the sixth day of week');
> +
> +is(issue_number($dt, 'week'), 37, '2022-09-17 is the week #37 of the year');
> +
> +is(issue_number($dt, 'month'), 9, '2017-09-17 is the ninth month of the year');

should be 2022-09-17 ...

Comment on attachment 70336 [details] [review]
Bug 17656 - Update irregularities on subscription renewals

Review of attachment 70336 [details] [review]:
-----------------------------------------------------------------

::: C4/Serials.pm
@@ +2713,4 @@
>      return @$serials;
>  }
>  
> +=head2 GessEnddate

typo

@@ +2718,5 @@
> +my $enddate = GessEnddate($startdate_iso $frequencyid, $numberlength, $weeklength, $monthlength);
> +
> +=cut
> +
> +sub GuessEnddate {

Better to add new subs just to Koha namespace... but I understand it here - we do not have all the objects needed for serials management (frequencies and number patterns are missing) and calling C4 subs from Koha namespace is not good idea... If you are not willing to add needed objects, we could just fill a bug for now...

::: Koha/Subscription.pm
@@ +77,5 @@
> +Return the length type (issues, weeks, months)
> +
> +=cut
> +
> +sub subtype {

Maybe lengthtype would be better name? Not blocker

::: koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-renew.tt
@@ +52,5 @@
> +        <input type="hidden" id="numbering3" value="[% numbering3 %]" />
> +        <input type="hidden" id="whenmorethan1" value="[% whenmorethan1 %]" />
> +        <input type="hidden" id="whenmorethan2" value="[% whenmorethan2 %]" />
> +        <input type="hidden" id="whenmorethan3" value="[% whenmorethan3 %]" />
> +        <input type="hidden" id="locale" value="[% locale %]" />

Why do you include all the data when renewing subscription? It should be not needed!

One more thing - you missed to add the new db column to kohastructure.sql. Also, the atomic update must be idempotent - you should always use IGNORE or helper subs like column_exists and so on - see https://wiki.koha-community.org/wiki/Database_updates

(In reply to Josef Moravec from comment #44)
> Comment on attachment 70336 [details] [review] [review]
> Bug 17656 - Update irregularities on subscription renewals
> 
> Review of attachment 70336 [details] [review] [review]:
> -----------------------------------------------------------------
>
> 
> Better to add new subs just to Koha namespace... but I understand it here -
> we do not have all the objects needed for serials management (frequencies
> and number patterns are missing) and calling C4 subs from Koha namespace is
> not good idea... If you are not willing to add needed objects, we could just
> fill a bug for now...

Don't understand. You mean create Koha::Subscription->GuessEnddate?
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-renew.tt
> @@ +52,5 @@
> > +        <input type="hidden" id="numbering3" value="[% numbering3 %]" />
> > +        <input type="hidden" id="whenmorethan1" value="[% whenmorethan1 %]" />
> > +        <input type="hidden" id="whenmorethan2" value="[% whenmorethan2 %]" />
> > +        <input type="hidden" id="whenmorethan3" value="[% whenmorethan3 %]" />
> > +        <input type="hidden" id="locale" value="[% locale %]" />
> 
> Why do you include all the data when renewing subscription? It should be not
> needed!

Yes. testPredictionPattern js function needs all the data.

Created attachment 70430 [details] [review]
Bug 17656 - Fix QA comments  - Make day and month names translatable,  - Update unit tests,  - Fix a typo in C4/Serials.pm (GessEndDate),  - rename subtype subroutine to lengthtype  - DB Change: add changes in kohastructure.sql and Bug_17656-add-permanent

Comment on attachment 70430 [details] [review]
Bug 17656 - Fix QA comments  - Make day and month names translatable,  - Update unit tests,  - Fix a typo in C4/Serials.pm (GessEndDate),  - rename subtype subroutine to lengthtype  - DB Change: add changes in kohastructure.sql and Bug_17656-add-permanent

Review of attachment 70430 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/includes/serials/subscription_daily_choice.inc
@@ +4,2 @@
>  </em></p>
>  [% daily_choice = [ {name => 'monday', desc => 'Monday', value => 1}, {name => 'tuesday', desc => 'Tuesday', value => 2}, {name => 'wednesday', desc => 'Wednesday', value => 3}, {name => 'thursday', desc => 'Thursday', value => 4}, {name => 'friday', desc => 'Friday', value => 5}, {name => 'saturday', desc => 'Saturday', value => 6}, {name => 'sunday', desc => 'Sunday', value => 7} ] %]

Looks like you do not need the "desc" values here now, is that true?

::: koha-tmpl/intranet-tmpl/prog/en/includes/serials/subscription_monthly_choice.inc
@@ +4,2 @@
>  </em></p>
>  [% monthly_choice = [ {name => 'january', desc => 'January', value => 1}, {name => 'february', desc => 'February', value => 2}, {name => 'march', desc => 'March', value => 3}, {name => 'april', desc => 'April', value => 4}, {name => 'may', desc => 'May', value => 5}, {name => 'june', desc => 'June', value => 6}, {name => 'july', desc => 'July', value => 7}, {name => 'august', desc => 'August', value => 8}, {name => 'september', desc => 'September', value => 9}, {name => 'october', desc => 'October', value => 10}, {name => 'november', desc => 'November', value => 11}, {name => 'december', desc => 'December', value =>12} ] %]

Looks like you do not need the "desc" values here now, is that true?

(In reply to Alex Arnaud from comment #46)
> (In reply to Josef Moravec from comment #44)
> > Comment on attachment 70336 [details] [review] [review] [review]
> > Bug 17656 - Update irregularities on subscription renewals
> > 
> > Review of attachment 70336 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> >
> > 
> > Better to add new subs just to Koha namespace... but I understand it here -
> > we do not have all the objects needed for serials management (frequencies
> > and number patterns are missing) and calling C4 subs from Koha namespace is
> > not good idea... If you are not willing to add needed objects, we could just
> > fill a bug for now...
> 
> Don't understand. You mean create Koha::Subscription->GuessEnddate?

Yes, that is what I ment.

> > 
> > ::: koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-renew.tt
> > @@ +52,5 @@
> > > +        <input type="hidden" id="numbering3" value="[% numbering3 %]" />
> > > +        <input type="hidden" id="whenmorethan1" value="[% whenmorethan1 %]" />
> > > +        <input type="hidden" id="whenmorethan2" value="[% whenmorethan2 %]" />
> > > +        <input type="hidden" id="whenmorethan3" value="[% whenmorethan3 %]" />
> > > +        <input type="hidden" id="locale" value="[% locale %]" />
> > 
> > Why do you include all the data when renewing subscription? It should be not
> > needed!
> 
> Yes. testPredictionPattern js function needs all the data.

Then I think the showpredictionpattern.pl script should be changed to use stored data for parameters which weren't changed...

(In reply to Josef Moravec from comment #49)
> (In reply to Alex Arnaud from comment #46)
> > (In reply to Josef Moravec from comment #44)
> > > Comment on attachment 70336 [details] [review] [review] [review] [review]
> > > Bug 17656 - Update irregularities on subscription renewals
> > > 
> > > Review of attachment 70336 [details] [review] [review] [review] [review]:
> > > -----------------------------------------------------------------
> > 
> > Yes. testPredictionPattern js function needs all the data.
> 
> Then I think the showpredictionpattern.pl script should be changed to use
> stored data for parameters which weren't changed...

Why? testPredictionPattern JS function get this data and pass them to showpredictionpattern.pl. It works exactly like in subscription-add.pl

(In reply to Josef Moravec from comment #49)
> (In reply to Alex Arnaud from comment #46)
> > (In reply to Josef Moravec from comment #44)
> > > Comment on attachment 70336 [details] [review] [review] [review] [review]
> > > Bug 17656 - Update irregularities on subscription renewals
> > > 
> > > Review of attachment 70336 [details] [review] [review] [review] [review]:
> > > -----------------------------------------------------------------
> > >
> > > 
> > > Better to add new subs just to Koha namespace... but I understand it here -
> > > we do not have all the objects needed for serials management (frequencies
> > > and number patterns are missing) and calling C4 subs from Koha namespace is
> > > not good idea... If you are not willing to add needed objects, we could just
> > > fill a bug for now...
> > 
> > Don't understand. You mean create Koha::Subscription->GuessEnddate?
> 
> Yes, that is what I ment.

I see one obstacle. GuessEnddate is used with the subscription's data itself in subscription-renew.pl and with data coming from the html form in subscription-add.pl. So method calls will be:

$subscription_object->GuessEnddate() in the first case,
and
$subscription_object->GuessEnddate($firstacquidate, $periodicity, ...) in the second one

Quite tricky no?

Created attachment 70619 [details] [review]
Bug 17656 - [QA followup] Remove useless description in subscription_daily_choice.inc and subscription_monthly_choice.inc

Alex, should this be set back to Signed off with your follow-up?

(In reply to Alex Arnaud from comment #51)
> (In reply to Josef Moravec from comment #49)
> > (In reply to Alex Arnaud from comment #46)
> > > (In reply to Josef Moravec from comment #44)
> > > > Comment on attachment 70336 [details] [review] [review] [review] [review] [review]
> > > > Bug 17656 - Update irregularities on subscription renewals
> > > > 
> > > > Review of attachment 70336 [details] [review] [review] [review] [review] [review]:
> > > > -----------------------------------------------------------------
> > > >
> > > > 
> > > > Better to add new subs just to Koha namespace... but I understand it here -
> > > > we do not have all the objects needed for serials management (frequencies
> > > > and number patterns are missing) and calling C4 subs from Koha namespace is
> > > > not good idea... If you are not willing to add needed objects, we could just
> > > > fill a bug for now...
> > > 
> > > Don't understand. You mean create Koha::Subscription->GuessEnddate?
> > 
> > Yes, that is what I ment.
> 
> I see one obstacle. GuessEnddate is used with the subscription's data itself
> in subscription-renew.pl and with data coming from the html form in
> subscription-add.pl. So method calls will be:
> 
> $subscription_object->GuessEnddate() in the first case,
> and
> $subscription_object->GuessEnddate($firstacquidate, $periodicity, ...) in
> the second one
> 
> Quite tricky no?

Not that tricky I think, you should use it like this:

my $subscription_object = Koha::Subscription->new( { firstacquidate => "....", periodicity => "....", ... });
my $enddate = $subscription_object->guess_end_date;

the call is always the same. It doesn't matter if it is in the database or not. If you wan't to save it to db, use ->store, but it is not an obligation.

Also, please consider to change ->irregularities and ->permanent_irregularities to be getter and setter.

It also will need rebase again, sorry.

Created attachment 75403 [details] [review]
Bug 17656 - Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for monthly, daily or weekly serials ( 1 unit per issue
only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Created attachment 75404 [details] [review]
Bug 17656 - Update irregularities on subscription renewals

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Renew subscription from 2017-10-16
  - Check that you have the prediction pattern table on renwal page
  - Submit renewal
  - Check that irregularity and permanent_irregularity column
    have been updated.

  - Do the same for a monthly and weekly subscription

Created attachment 75405 [details] [review]
Bug 17656 - Fix QA comments - Make day and month names translatable, - Update unit tests, - Fix a typo in C4/Serials.pm (GessEndDate), - rename subtype subroutine to lengthtype - DB Change: add changes in kohastructure.sql and Bug_17656-add-permanent-irre

Created attachment 75406 [details] [review]
Bug 17656 - [QA followup] Remove useless description in subscription_daily_choice.inc and subscription_monthly_choice.inc

Created attachment 75407 [details] [review]
Bug 17656: Update irregularities on multi-renewals

Created attachment 75408 [details] [review]
Bug 17656: move GuessEnddate in Koha::Subscription::guess_enddate

I've rebased these patches. Particulary upon bug 7910 that has been pushed recently. SO, addition for the plan is:

  - create several subscription with permanent irregularities,
  - renew them using the multi-renew feature,
  - check that the new irregulaties are consistent

Bonjour Alex,
J'étais parti, le coeur plein d'entrain, pour une nouvelle séance de test sur la nouvelle mouture de ton patch (tu as dû te régaler..), mais après mise à jour d'une master de test que j'ai sur vbox (qu'avait pas servi depuis la dernière fois), je me retrouve avec un affichage déglingué et je constate que koha cherche à charger les css et js en leur ajoutant un suffixe
18.0500000 (ça ressemble à la date de mise à jour du patch ?!) :
http://preprod:8080/intranet-tmpl/prog/css/staff-global_18.0500000.css 
Soit j'ai fait une grosse bourde, soit il y a quelque chose qui m'échappe (voire même les deux d'ailleurs ). ça te parle ? 

Olivier

(In reply to Koha Team Lyon 3 from comment #63)
> Bonjour Alex,
> J'étais parti, le coeur plein d'entrain, pour une nouvelle séance de test
> sur la nouvelle mouture de ton patch (tu as dû te régaler..), mais après
> mise à jour d'une master de test que j'ai sur vbox (qu'avait pas servi
> depuis la dernière fois), je me retrouve avec un affichage déglingué et je
> constate que koha cherche à charger les css et js en leur ajoutant un suffixe
> 18.0500000 (ça ressemble à la date de mise à jour du patch ?!) :
> http://preprod:8080/intranet-tmpl/prog/css/staff-global_18.0500000.css 
> Soit j'ai fait une grosse bourde, soit il y a quelque chose qui m'échappe
> (voire même les deux d'ailleurs ). ça te parle ? 
> 
> Olivier

Hello Olivier,

You have to update web server's rewrite rules. See bug 20187.
For apache, it is:

RewriteRule ^(.*)_[0-9][0-9]\.[0-9][0-9][0-9][0-9][0-9][0-9][0-9].js$ $1.js [L]
RewriteRule ^(.*)_[0-9][0-9]\.[0-9][0-9][0-9][0-9][0-9][0-9][0-9].css$ $1.css [L]

Nginx:

rewrite     "^(.*)_[0-9]{2}\.[0-9]{7}\.(js|css)$" $1.$2;

Great ! that's much better with the js ans css rewriting rules !
I could did the test again for daily, weekly and monthly periodicities. Everything is working as expected, dates are correctly skipped, permanent irregularities are kept over the renewal. 
Just a little thing about ergonomy of "Week(s) not published" scrolldown list : when you tick/untick a checkbox at first time, the form jumps surprisingly.
Could we avoid this ?

Anyway, thank you Alex and I hope that it will be OK this time.

Olivier

Created attachment 75721 [details] [review]
Bug 17656 - Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for monthly, daily or weekly serials ( 1 unit per issue
only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Created attachment 75722 [details] [review]
Bug 17656 - Update irregularities on subscription renewals

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Renew subscription from 2017-10-16
  - Check that you have the prediction pattern table on renwal page
  - Submit renewal
  - Check that irregularity and permanent_irregularity column
    have been updated.

  - Do the same for a monthly and weekly subscription

Created attachment 75723 [details] [review]
Bug 17656 - Fix QA comments - Make day and month names translatable, - Update unit tests, - Fix a typo in C4/Serials.pm (GessEndDate), - rename subtype subroutine to lengthtype - DB Change: add changes in kohastructure.sql and Bug_17656-add-permanent-irre

Created attachment 75724 [details] [review]
Bug 17656 - [QA followup] Remove useless description in subscription_daily_choice.inc and subscription_monthly_choice.inc

Created attachment 75725 [details] [review]
Bug 17656: Update irregularities on multi-renewals

Created attachment 75726 [details] [review]
Bug 17656: move GuessEnddate in Koha::Subscription::guess_enddate

(In reply to Koha Team Lyon 3 from comment #65)
> Just a little thing about ergonomy of "Week(s) not published" scrolldown
> list : when you tick/untick a checkbox at first time, the form jumps
> surprisingly.
> Could we avoid this ?
> 
Fixed!

I went through another round of testing. Looking good now, just one (hopefully last) issue: 
When the subscription is based on week, you can check irregularities for all pregenerated issues - but it works only when you check weeks from first year.

Probably there should be just week numbers from 1 to 52, which could be enough.

(In reply to Josef Moravec from comment #73)
> I went through another round of testing. Looking good now, just one
> (hopefully last) issue: 
> When the subscription is based on week, you can check irregularities for all
> pregenerated issues - but it works only when you check weeks from first year.
> 
> Probably there should be just week numbers from 1 to 52, which could be
> enough.

Don't understand. Can you give me an example ?

(In reply to Alex Arnaud from comment #74)
> (In reply to Josef Moravec from comment #73)
> > I went through another round of testing. Looking good now, just one
> > (hopefully last) issue: 
> > When the subscription is based on week, you can check irregularities for all
> > pregenerated issues - but it works only when you check weeks from first year.
> > 
> > Probably there should be just week numbers from 1 to 52, which could be
> > enough.
> 
> Don't understand. Can you give me an example ?

Here is the screenshot:

https://screenshots.firefox.com/hOUt987Krzxs6uTm/localhost

the other years weeks are not working - they do not set the checkboxes on previewed issues

Created attachment 75953 [details] [review]
Bug 17656: predication table - check all issues with same data-dow

Hello Alex

I tested again but I don't see any change about what Joseph Moravec told about nor about jumping checkboxes.

Am I missing something ?

Olivier

(In reply to Koha Team Lyon 3 from comment #77)
> Hello Alex
> 
> I tested again but I don't see any change about what Joseph Moravec told
> about nor about jumping checkboxes.
> 
> Am I missing something ?
> 
> Olivier

Hello Olivier,

The problem Joseph talked about as i understood it is that clicking a permanent irregularity does not check all the correponding irregularities. It checks only the first one in fact. 

My last fix works well for me but maybe i still missunderstand anything.
Joseph, can you confirm ?

For the "jumping checkboxes", i did not manage to reproduce it. Can you remind me where and how it happens ?

What I understood about Joseph comment is what follows :
Say we configure a weekly review from 01/01/2018 to 31/12/2018,if you set 52 issues as subscription length, it will be okay.
A priori, it makes sense that you will set permanent irregularities on a single cycle/subscription (but people are strange !).
If, for some reason, you put, say, 56 issues as subscription length, you will obtain as much checkboxes in the checkboxes lists.
In the "weeks not published" scrolldown list, you will have four extra checkboxes (corresponding to weeks 1,2,3,4 of year 2019).
If you check one of these weeks, the corresponding checkbox will not be checked on the fixed list and you risk not to notice this.
It's the reason why, I guess, Joseph suggests to limit the displayed list to 52 weeks (Some year may have 53 weeks according to iso numbering. may be we can limit by end date of current year. Problem, I'm thinking that some subscription (and numbering)  begin in the middle of the year and finish the year after like New York Review.

as for "jumping checkboxes", It occurs in the "weeks not published" scrolldown list and only at first checkbox checking. the second checking you do, no problem.

Sorry the patches could not apply, can you please rebase

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17656 - Keep irregularities during subscription renewals
Using index info to reconstruct a base tree...
M	C4/Serials.pm
M	koha-tmpl/intranet-tmpl/prog/css/staff-global.css
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/showpredictionpattern.tt
M	serials/subscription-add.pl
Falling back to patching base and 3-way merge...
Auto-merging serials/subscription-add.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/showpredictionpattern.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/css/staff-global.css
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/css/staff-global.css
Auto-merging C4/Serials.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 17656 - Keep irregularities during subscription renewals
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-17656---Keep-irregularities-during-subscriptio-4ETfMG.patch

Major bug, can we please get a rebase?

(In reply to Katrin Fischer from comment #81)
> Major bug, can we please get a rebase?

Still waiting and hoping for a rebase...

Created attachment 102724 [details] [review]
Bug 17656 - Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for monthly, daily or weekly serials ( 1 unit per issue
only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Created attachment 102725 [details] [review]
Bug 17656 - Update irregularities on subscription renewals

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Renew subscription from 2017-10-16
  - Check that you have the prediction pattern table on renwal page
  - Submit renewal
  - Check that irregularity and permanent_irregularity column
    have been updated.

  - Do the same for a monthly and weekly subscription

Created attachment 102726 [details] [review]
Bug 17656 - Fix QA comments - Make day and month names translatable, - Update unit tests, - Fix a typo in C4/Serials.pm (GessEndDate), - rename subtype subroutine to lengthtype - DB Change: add changes in kohastructure.sql and Bug_17656-add-permanent-irre

Created attachment 102727 [details] [review]
Bug 17656 - [QA followup] Remove useless description in subscription_daily_choice.inc and subscription_monthly_choice.inc

Created attachment 102728 [details] [review]
Bug 17656: Update irregularities on multi-renewals

Created attachment 102729 [details] [review]
Bug 17656: move GuessEnddate in Koha::Subscription::guess_enddate

Created attachment 102730 [details] [review]
Bug 17656: predication table - check all issues with same data-dow

(In reply to Katrin Fischer from comment #82)
> (In reply to Katrin Fischer from comment #81)
> > Major bug, can we please get a rebase?
> 
> Still waiting and hoping for a rebase...

Patches rebased

Created attachment 102860 [details] [review]
Bug 17656: Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for daily, weekly and monthly serials (1 unit
per issue only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Created attachment 102861 [details] [review]
Bug 17656: Update irregularities on subscription renewals

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Renew subscription from 2017-10-16
  - Check that you have the prediction pattern table on renwal page
  - Submit renewal
  - Check that irregularity and permanent_irregularity column
    have been updated.

  - Do the same for a monthly and weekly subscription

Created attachment 102862 [details] [review]
Bug 17656: Fix issues found by QA

- Make day and month names translatable
- Update unit tests
- Fix a typo in C4/Serials.pm (GessEndDate)
- Rename subtype subroutine to lengthtype
- DB Change: add changes in kohastructure.sql
  and Bug_17656-add-permanent-irregularities.perl

Created attachment 102863 [details] [review]
Bug 17656: (QA follow-up) Remove useless description in subscription_daily_choice.inc and subscription_monthly_choice.inc

Created attachment 102864 [details] [review]
Bug 17656: Update irregularities on multi-renewals

Created attachment 102865 [details] [review]
Bug 17656: Move GuessEnddate in Koha::Subscription::guess_enddate

Created attachment 102866 [details] [review]
Bug 17656: Predication table - check all issues with same data-dow

Created attachment 102867 [details] [review]
Bug 17656: (follow-up) Add missing filters

Hi Alex,

I've cleaned up the commit messages and added the missing filters to make the QA tools happy, but I got 2 remaining issues in the QA script:

 FAIL	serials/subscription-renew.pl
   FAIL	  valid
		"my" variable $subscription masks earlier declaration in same scope 

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/serials/showpredictionpattern.tt
   OK	  filters
   FAIL	  forbidden patterns
		forbidden pattern: Attribute type should not be used for script tags (bug 20053) (line 9)

Could you have a look at those please?

Created attachment 102960 [details] [review]
Bug 17656: Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for daily, weekly and monthly serials (1 unit
per issue only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Created attachment 102961 [details] [review]
Bug 17656: Update irregularities on subscription renewals

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Renew subscription from 2017-10-16
  - Check that you have the prediction pattern table on renwal page
  - Submit renewal
  - Check that irregularity and permanent_irregularity column
    have been updated.

  - Do the same for a monthly and weekly subscription

Created attachment 102962 [details] [review]
Bug 17656: Fix issues found by QA

- Make day and month names translatable
- Update unit tests
- Fix a typo in C4/Serials.pm (GessEndDate)
- Rename subtype subroutine to lengthtype
- DB Change: add changes in kohastructure.sql
  and Bug_17656-add-permanent-irregularities.perl

Created attachment 102963 [details] [review]
Bug 17656: (QA follow-up) Remove useless description in subscription_daily_choice.inc and subscription_monthly_choice.inc

Created attachment 102964 [details] [review]
Bug 17656: Update irregularities on multi-renewals

Created attachment 102965 [details] [review]
Bug 17656: Move GuessEnddate in Koha::Subscription::guess_enddate

Created attachment 102966 [details] [review]
Bug 17656: Predication table - check all issues with same data-dow

Created attachment 102967 [details] [review]
Bug 17656: (follow-up) Add missing filters

Created attachment 102968 [details] [review]
Bug 17656: fix QA tools warnings

(In reply to Katrin Fischer from comment #99)
> Hi Alex,
> 
> I've cleaned up the commit messages and added the missing filters to make
> the QA tools happy, but I got 2 remaining issues in the QA script:
> 
>  FAIL	serials/subscription-renew.pl
>    FAIL	  valid
> 		"my" variable $subscription masks earlier declaration in same scope 
> 
>  FAIL
> koha-tmpl/intranet-tmpl/prog/en/modules/serials/showpredictionpattern.tt
>    OK	  filters
>    FAIL	  forbidden patterns
> 		forbidden pattern: Attribute type should not be used for script tags (bug
> 20053) (line 9)
> 
> Could you have a look at those please?

Katrin,

QA warnings are fixed.
Also, i did a tiny rebase again.

Thx!

Patch no longer applies 8-(:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17656: Keep irregularities during subscription renewals
Using index info to reconstruct a base tree...
M	C4/Serials.pm
M	koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss
M	koha-tmpl/intranet-tmpl/prog/css/staff-global.css
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/css/staff-global.css
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/css/staff-global.css
Auto-merging koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss
Auto-merging C4/Serials.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 17656: Keep irregularities during subscription renewals

Created attachment 103623 [details] [review]
Bug 17656: Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for daily, weekly and monthly serials (1 unit
per issue only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Created attachment 103624 [details] [review]
Bug 17656: Update irregularities on subscription renewals

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Renew subscription from 2017-10-16
  - Check that you have the prediction pattern table on renwal page
  - Submit renewal
  - Check that irregularity and permanent_irregularity column
    have been updated.

  - Do the same for a monthly and weekly subscription

Created attachment 103625 [details] [review]
Bug 17656: Fix issues found by QA

- Make day and month names translatable
- Update unit tests
- Fix a typo in C4/Serials.pm (GessEndDate)
- Rename subtype subroutine to lengthtype
- DB Change: add changes in kohastructure.sql
  and Bug_17656-add-permanent-irregularities.perl

Created attachment 103626 [details] [review]
Bug 17656: (QA follow-up) Remove useless description in subscription_daily_choice.inc and subscription_monthly_choice.inc

Created attachment 103627 [details] [review]
Bug 17656: Update irregularities on multi-renewals

Created attachment 103628 [details] [review]
Bug 17656: Move GuessEnddate in Koha::Subscription::guess_enddate

Created attachment 103629 [details] [review]
Bug 17656: Predication table - check all issues with same data-dow

Created attachment 103630 [details] [review]
Bug 17656: (follow-up) Add missing filters

Created attachment 103631 [details] [review]
Bug 17656: fix QA tools warnings

(In reply to David Nind from comment #110)
> Patch no longer applies 8-(:
> 
> Apply? [(y)es, (n)o, (i)nteractive] y
> Applying: Bug 17656: Keep irregularities during subscription renewals
> Using index info to reconstruct a base tree...
> M	C4/Serials.pm
> M	koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss
> M	koha-tmpl/intranet-tmpl/prog/css/staff-global.css
> Falling back to patching base and 3-way merge...
> Auto-merging koha-tmpl/intranet-tmpl/prog/css/staff-global.css
> CONFLICT (content): Merge conflict in
> koha-tmpl/intranet-tmpl/prog/css/staff-global.css
> Auto-merging koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss
> Auto-merging C4/Serials.pm
> error: Failed to merge in the changes.
> Patch failed at 0001 Bug 17656: Keep irregularities during subscription
> renewals

Patches rebased

Created attachment 103657 [details] [review]
Bug 17656: Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for daily, weekly and monthly serials (1 unit
per issue only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 103658 [details] [review]
Bug 17656: Update irregularities on subscription renewals

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Renew subscription from 2017-10-16
  - Check that you have the prediction pattern table on renwal page
  - Submit renewal
  - Check that irregularity and permanent_irregularity column
    have been updated.

  - Do the same for a monthly and weekly subscription

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 103659 [details] [review]
Bug 17656: Fix issues found by QA

- Make day and month names translatable
- Update unit tests
- Fix a typo in C4/Serials.pm (GessEndDate)
- Rename subtype subroutine to lengthtype
- DB Change: add changes in kohastructure.sql
  and Bug_17656-add-permanent-irregularities.perl

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 103660 [details] [review]
Bug 17656: (QA follow-up) Remove useless description in subscription_daily_choice.inc and subscription_monthly_choice.inc

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 103661 [details] [review]
Bug 17656: Update irregularities on multi-renewals

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 103662 [details] [review]
Bug 17656: Move GuessEnddate in Koha::Subscription::guess_enddate

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 103663 [details] [review]
Bug 17656: Predication table - check all issues with same data-dow

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 103664 [details] [review]
Bug 17656: (follow-up) Add missing filters

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 103665 [details] [review]
Bug 17656: fix QA tools warnings

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 103667 [details]
Bug 17656 - Image of renewal form

(In reply to Alex Arnaud from comment #120)
 
> Patches rebased

Thanks Alex!

Everything seems to work as per the test plan.

I've signed-off, but I'm not sure that the renewal form displays correctly - see screenshot attached.

Looking here

 FAIL   installer/data/mysql/atomicupdate/Bug_17656-add-permanent-irregularities.sql
   FAIL   git manipulation
                The file has been added and deleted in the same patchset

-    $periodicity, $firstacquidate, $irregularity, $numberpattern, $locale,
-    $numberlength, $weeklength, $monthlength, $lastvalue1, $innerloop1,
-    $lastvalue2, $innerloop2, $lastvalue3, $innerloop3, $status,
-    $biblionumber, $callnumber, $notes, $letter, $manualhistory,
-    $internalnotes, $serialsadditems, $staffdisplaycount, $opacdisplaycount,
-    $graceperiod, $location, $enddate, $subscriptionid, $skip_serialseq,
-    $itemtype, $previousitemtype, $mana_id
+    $periodicity, $firstacquidate, $irregularity, $permanent_irregularity,
+    $numberpattern, $locale, $numberlength, $weeklength, $monthlength,
+    $lastvalue1, $innerloop1, $lastvalue2, $innerloop2, $lastvalue3,
+    $innerloop3, $status, $biblionumber, $callnumber, $notes, $letter,
+    $manualhistory, $internalnotes, $serialsadditems, $staffdisplaycount,
+    $opacdisplaycount, $graceperiod, $location, $enddate, $subscriptionid,
+    $skip_serialseq, $itemtype, $previousitemtype, $mana_id

Doing things like this does not life make easier for QA.
Do I now have to check if every parameter is still there?
You could have added the permanent_irregularity without touching all the other lines. And you can still fix it too?

Same remark for this:

-            librarian         => $auser,
-            branchcode        => $branchcode,
-            aqbooksellerid    => $aqbooksellerid,
-            cost              => $cost,
-            aqbudgetid        => $aqbudgetid,
-            biblionumber      => $biblionumber,
-            startdate         => $startdate,
-            periodicity       => $periodicity,
-            numberlength      => $numberlength,
-            weeklength        => $weeklength,
-            monthlength       => $monthlength,
-            lastvalue1        => $lastvalue1,
-            innerloop1        => $innerloop1,
-            lastvalue2        => $lastvalue2,
-            innerloop2        => $innerloop2,
-            lastvalue3        => $lastvalue3,
-            innerloop3        => $innerloop3,
-            status            => $status,
-            notes             => $notes,
-            letter            => $letter,
-            firstacquidate    => $firstacquidate,
-            irregularity      => $irregularity,
-            numberpattern     => $numberpattern,
-            locale            => $locale,
-            callnumber        => $callnumber,
-            manualhistory     => $manualhistory,
-            internalnotes     => $internalnotes,
-            serialsadditems   => $serialsadditems,
-            staffdisplaycount => $staffdisplaycount,
-            opacdisplaycount  => $opacdisplaycount,
-            graceperiod       => $graceperiod,
-            location          => $location,
-            enddate           => $enddate,
-            skip_serialseq    => $skip_serialseq,
-            itemtype          => $itemtype,
-            previousitemtype  => $previousitemtype,
-            mana_id           => $mana_id,
+            librarian               => $auser,
+            branchcode              => $branchcode,
+            aqbooksellerid          => $aqbooksellerid,
+            cost                    => $cost,
+            aqbudgetid              => $aqbudgetid,
+            biblionumber            => $biblionumber,
+            startdate               => $startdate,
+            periodicity             => $periodicity,
+            numberlength            => $numberlength,

etc

+=head2 GessEnddate
+
+my $enddate = GessEnddate($startdate_iso $frequencyid, $numberlength, $weeklength, $monthlength);
+
+=cut
+
+sub GuessEnddate {

The routine name should obviously be the same in POD and code.

(In reply to Marcel de Rooy from comment #135)
> +=head2 GessEnddate
> +
> +my $enddate = GessEnddate($startdate_iso $frequencyid, $numberlength,
> $weeklength, $monthlength);
> +
> +=cut
> +
> +sub GuessEnddate {
> 
> The routine name should obviously be the same in POD and code.

Seeing a correction now. Reason for some squashing ?

You add a routine to DateUtils called issue_number:

+my $week_number = issue_number('2017-01-01', 'week');

What you actually want is a day-of-week, week number or month.

The name is confusing. I do not expect an issue_number in DateUtils.
The POD is wrong since you expect a dt and raise an exception on a string.

+ return Day_of_Week(split /-/, $date->ymd);
Please consider using a DateTime function here:
$dt->day_of_week()
$dt->wday() and $dt->dow().
$dt->local_day_of_week()
Actually $dt->dow is shorter than issue_number($dt, 'week') 

+ return $date->month;
In this case it is more work to call issue_number($dt, 'month') than to say $dt->month.

+    my $dow = issue_number( dt_from_string($date), $frequency->unit );
This is a bit confusing. If we do not know the unit, how could we save it into dow (day of week) ? Rename ?

I recommend to remove issue_number from DateUtils. We could move it to Serials? It is a thin wrapper to three DateTime methods. If you want to keep it in DateUtils, we should come up with a convincing name.

+my $subscription_o = Koha::Subscriptions->find($subscription->{subscriptionid});
+my @irreg = $subscription_o->irregularities;
+is($irreg[0], 2, 'First element of irregularities is 2');
+is($irreg[1], 4, 'Second element of irregularities is 4');
+is($irreg[2], 7, 'Third element of irregularities is 7');
+
+my @permanent_irreg = $subscription_o->permanent_irregularities;
+is($permanent_irreg[0], 1, 'First element of permanent_irregularities is 1');
+is($permanent_irreg[1], 2, 'Second element of permanent_irregularities is 2');

Do we really need this?
You add a separate script here to test if split /;/  works ?
I appreciate your willingness to test, but I have the impression that we should focus here on complex exceptions with various frequencies, different months, leap years, etc. Think of various date peculiarities.

Elaborating on that, t/db_dependent/Koha/Subscription/Irregularities.t contains a lot of code, but actually only one test.
+ is($irregularities, '10;11', 'Irregularities have been updated');
Since this looks like the most important test, I would really expect more testing here. Maybe you should think of some intelligent mocks here to reduce a lot of building calls?

Since we already got into string freeze, I leave it for now. Please resubmit your work in some squashed way. Perhaps divide the module changes, tests, script and template changes, and db revisions over separate patches.

Thanks !

Created attachment 117789 [details] [review]
Bug 17656: Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for daily, weekly and monthly serials (1 unit
per issue only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Rebased - 2021-03-04 - Alex Arnaud <alex.arnaud@biblibre.com>

Created attachment 117790 [details] [review]
Bug 17656: (follow-up) Add missing filters

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 117791 [details] [review]
Bug 17656: [DO NOT PUSH] provide schema file

Created attachment 117792 [details] [review]
Bug 17656: [DO NOT PUSH] provide css file

Patches rebased and squashed.

@Marcel, I took your QA remarks into account.

CSS is already conflicting - I will leave this patch off for testing running yarn build locally.

I am sorry, but there are quite some QA fails here :(

It looks like the first could be a rebase issue?

 FAIL	C4/Serials.pm
   FAIL	  valid
		Constant subroutine C4::Serials::MISSING_LOST redefined 
		Constant subroutine C4::Serials::MISSING_NEVER_RECIEVED redefined 
		Constant subroutine C4::Serials::STOPPED redefined 
		Constant subroutine C4::Serials::DELETED redefined 
		Constant subroutine C4::Serials::MISSING_DAMAGED redefined 
		Constant subroutine C4::Serials::EXPECTED redefined 
		Constant subroutine C4::Serials::LATE redefined 
		Constant subroutine C4::Serials::MISSING_SOLD_OUT redefined 
		Constant subroutine C4::Serials::ARRIVED redefined 
		Constant subroutine C4::Serials::NOT_ISSUED redefined 
		Constant subroutine C4::Serials::CLAIMED redefined 
		Constant subroutine C4::Serials::MISSING redefined 

 FAIL	t/Serials/IssueNumber.t
   OK	  critic
   FAIL	  file permissions
		File must have the exec flag

 FAIL	t/db_dependent/Koha/Subscription/GuessEnddate.t
   OK	  critic
   FAIL	  file permissions
		File must have the exec flag


 FAIL	t/db_dependent/Koha/Subscription/Irregularities.t
   OK	  critic
   FAIL	  file permissions
		File must have the exec flag

 FAIL	t/db_dependent/Serials/SubscriptionIrregularity.t
   OK	  critic
   FAIL	  file permissions
		File must have the exec flag

Created attachment 117846 [details] [review]
Bug 17656: [DO NOT PUSH] provide css file

Created attachment 117852 [details] [review]
Bug 17656: QA warnings fixes

Created attachment 118856 [details] [review]
Bug 17656: QA warnings fixes

Signed-off-by: Amarangi  <amarangikarunathilaka@gmail.com>

Hello Alex,
Sorry but when I record a subscription after adding irregularities via Test prediction pattern and I click on Save Subscription button, I get this error :

The method Koha::Subscriptions->startdate is not covered by tests!

I tried to create a new subs and that time it was OK.
But when I create another sub for testing another type of frequency I got this error again.

Olivier Crouzet

Some more details  :

I tried a daily frequency with the following data :

05/04/2021 to 11/04/2021 (french notation = 5 april 2021 to 11 april 2021)

first issue 05/04/2021
subs start : idem
issues 7
subs end : 11/04/2021
wednesday and thursday are checked as irregulars
multireceiving 5 items
renewing :  wednesday and thursday are still checked. That's OK but when I submit the form, the following message is displayed : "Renewing is cancelled, contact the administrator"
Going back to Editing subscription, in  Test pattern it's said :
End date is not consistent with subscription length.
Indeed end date is remained on 11/04/2021
I decided to delete enddate and let it making itself but when saving I get again the error message "The method Koha::Subscriptions->startdate is not covered by tests!"

Olivier

Created attachment 119106 [details] [review]
Bug 17656: use Koha::Subscription->startdate instead of Koha::Subscriptions

Hello,
I testes this time on Ptfx Sandbox. no more message "Renewal cancelled". Sorry, I think this was template problem on my instance.

It's OK now for startdate error message.

Everything works as expected as for daily and monthly frequencies.
Exceptions are kept. date are skipped when receiving as well.

But unfortunately weekly frequency does not work. Irregularities are not retained :
my parameters :
first issue publ. date  01/01/2021
startdate 01/01/2021
issues 52
enddate 31/12/2021

I checked 8 and 15 feb 2021
They are recorded ok
I multireceived 50 issues and click on Renew button but no weeks are no more checked on the form.

Olivier

Thx for all the testing going on here!

(In reply to Koha Team University Lyon 3 from comment #154)
> 
> I checked 8 and 15 feb 2021
> They are recorded ok
> I multireceived 50 issues and click on Renew button but no weeks are no more
> checked on the form.
> 
> Olivier

Olivier,

Did you check the weeks in the list above the prediction table ?
Doing so works for me.

If you check weeks inside the prediction table (column "Not published"), they are not saved as permanent irregularities but as irregularities.

Oh ! stupid ! I did'nt get it !. I think I only checked the flat list below.
I redo the test checking the irregularities in the choice list and you are right it-s OK : this time irregularities are kept.

I will sign off but I have 2 last suggestions :

I think all the same, the display for weeks type is little ambiguous :
The presence of 2 wordings very close can be troubling

Please check the issues that are NOT published (irregularities)
and below :
Choice the weeks in the year where the issues are not published

May be, in this case, the second is redundant. I would suggest to suppress the second and make the first like this :
Please check in the dropdown list, the issues that are NOT published (irregularities)

and last thing : the "Weeks(s)not_published" form is jumping when we check a checkbox. Could you add a "return false" to "Check_permanent_irregularities" function to prevent this ?

Olivier

Created attachment 120863 [details] [review]
Bug 17656: More meaningful messages in prediction pattern table

(In reply to Koha Team University Lyon 3 from comment #157)
> Please check the issues that are NOT published (irregularities)
> and below :
> Choice the weeks in the year where the issues are not published
> 
> May be, in this case, the second is redundant. I would suggest to suppress
> the second and make the first like this :
> Please check in the dropdown list, the issues that are NOT published
> (irregularities)
> 
> and last thing : the "Weeks(s)not_published" form is jumping when we check a
> checkbox. Could you add a "return false" to "Check_permanent_irregularities"
> function to prevent this ?
> 
> Olivier


Olivier,

Please test my proposal for meaningful messages.

Also, fixed the jump on top of the page.

Hey Alex,

Great !
That's perfect.

Hope this patch will be in master soon, what's the english for "je croise les doigt" ? fingers crossed ?


Olivier

(In reply to Koha Team University Lyon 3 from comment #160)
> Hope this patch will be in master soon, what's the english for "je croise
> les doigt" ? fingers crossed ?

yes ^^"

Too bad I hit an issue when testing however :o


(In reply to Koha Team University Lyon 3 from comment #151)
> The method Koha::Subscriptions->startdate is not covered by tests!

same!

details
- rebuilt schema files[1]
- new subscription
- record 146 from sample data
- first publication date: 05/02/2021
- Frequency 1/day
- start date 05/02/2021
- numbering pattern number
- Subscription length: 7
- not published:  05/04/2021 &  05/05/2021
- save

The method Koha::Subscriptions->startdate is not covered by tests!

[1] dbic command from koha-testing-docker
mysql -h db -ppassword -uroot -e 'DROP DATABASE IF EXISTS dbic; CREATE DATABASE dbic'
cd '/kohadevbox/koha'
mysql -h db -ppassword -uroot dbic < installer/data/mysql/kohastructure.sql
misc/devel/update_dbix_class_files.pl --db_name dbic --db_host db --db_user root --db_passwd password


Did I do something wrong?
Reverted to "need signoff" so it's not in the signoff queue. Which would cause someone from QA to look at it, pass it and don't look a it later.

Created attachment 121082 [details] [review]
Bug 17656: Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for daily, weekly and monthly serials (1 unit
per issue only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Rebased - 2021-03-04 - Alex Arnaud <alex.arnaud@biblibre.com>

Created attachment 121083 [details] [review]
Bug 17656: (follow-up) Add missing filters

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 121084 [details] [review]
Bug 17656: [DO NOT PUSH] provide schema file

Created attachment 121085 [details] [review]
Bug 17656: [DO NOT PUSH] provide css file

Created attachment 121086 [details] [review]
Bug 17656: QA warnings fixes

Signed-off-by: Amarangi  <amarangikarunathilaka@gmail.com>

Created attachment 121087 [details] [review]
Bug 17656: use Koha::Subscription->startdate instead of Koha::Subscriptions

Created attachment 121088 [details] [review]
Bug 17656: More meaningful messages in prediction pattern table

(In reply to Victor Grousset/tuxayo from comment #161)
> 
> The method Koha::Subscriptions->startdate is not covered by tests!
> 

Victor, it's ok now. I squashed the fix in "use Koha::Subscription->startdate instead of Koha::Subscriptions" patch. It's the same problem in other place.

Patches rebased.

It works :) Thanks for the fix

Created attachment 121296 [details] [review]
Bug 17656: Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for daily, weekly and monthly serials (1 unit
per issue only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Rebased - 2021-03-04 - Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121297 [details] [review]
Bug 17656: (follow-up) Add missing filters

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121298 [details] [review]
Bug 17656: [DO NOT PUSH] provide schema file

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121299 [details] [review]
Bug 17656: [DO NOT PUSH] provide css file

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121300 [details] [review]
Bug 17656: QA warnings fixes

Signed-off-by: Amarangi  <amarangikarunathilaka@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121301 [details] [review]
Bug 17656: use Koha::Subscription->startdate instead of Koha::Subscriptions

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121302 [details] [review]
Bug 17656: More meaningful messages in prediction pattern table

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

For some reason not all commits had the signed off mark. I confirm having tested with all applied.

Adds unused function _get_sub_length which should be removed.

Doesn't apply, please remove unused function in comment 186

Created attachment 128489 [details] [review]
Bug 17656: Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for daily, weekly and monthly serials (1 unit
per issue only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Rebased - 2021-03-04 - Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128491 [details] [review]
Bug 17656: (follow-up) Add missing filters

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128492 [details] [review]
Bug 17656: [DO NOT PUSH] provide schema file

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128493 [details] [review]
Bug 17656: [DO NOT PUSH] provide css file

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128494 [details] [review]
Bug 17656: QA warnings fixes

Signed-off-by: Amarangi  <amarangikarunathilaka@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128495 [details] [review]
Bug 17656: use Koha::Subscription->startdate instead of Koha::Subscriptions

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128496 [details] [review]
Bug 17656: More meaningful messages in prediction pattern table

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128498 [details] [review]
Bug 17656: Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for daily, weekly and monthly serials (1 unit
per issue only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Rebased - 2021-03-04 - Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128499 [details] [review]
Bug 17656: (follow-up) Add missing filters

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128500 [details] [review]
Bug 17656: [DO NOT PUSH] provide schema file

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128501 [details] [review]
Bug 17656: [DO NOT PUSH] provide css file

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128502 [details] [review]
Bug 17656: QA warnings fixes

Signed-off-by: Amarangi  <amarangikarunathilaka@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128503 [details] [review]
Bug 17656: use Koha::Subscription->startdate instead of Koha::Subscriptions

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 128504 [details] [review]
Bug 17656: More meaningful messages in prediction pattern table

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Patchs rebased

Patch doesn't apply :

<pre>
root@kohadevbox:koha(bz17656)$ git bz apply 17656

Bug 17656 - Irregularities in serial prediction pattern are planned only for current subscription

128498 - Bug 17656: Keep irregularities during subscription renewals
128499 - Bug 17656: (follow-up) Add missing filters
128500 - Bug 17656: [DO NOT PUSH] provide schema file
128501 - Bug 17656: [DO NOT PUSH] provide css file
128502 - Bug 17656: QA warnings fixes
128503 - Bug 17656: use Koha::Subscription->startdate instead of Koha::Subscriptions
128504 - Bug 17656: More meaningful messages in prediction pattern table

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17656: Keep irregularities during subscription renewals
Using index info to reconstruct a base tree...
M	C4/Serials.pm
M	installer/data/mysql/kohastructure.sql
M	koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt
M	koha-tmpl/intranet-tmpl/prog/js/subscription-add.js
M	serials/subscription-add.pl
Falling back to patching base and 3-way merge...
Auto-merging serials/subscription-add.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/js/subscription-add.js
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/subscription-add.js
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Serials.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 17656: Keep irregularities during subscription renewals
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-17656-Keep-irregularities-during-subscription--JneHV9.patch

</pre>

Created attachment 132412 [details] [review]
Bug 17656: Keep irregularities during subscription renewals

When renewing a subscription, irregularities are lost. This patch
allows to keep them for daily, weekly and monthly serials (1 unit
per issue only).

Test plan:
  - Apply this patch,
  - Launch installer/data/mysql/updatedatabase.pl
  - Launch misc/devel/update_dbix_class_files.pl

  - Create a daily subscription. i.e:
    - From: 2017-10-09, To: 2017-10-15,
    - Frequency: 1/day,
    - 7 issues,
    - test prediction pattern
    - check Wednesday and Thursday as irregularity,
    - save
  - Edit subscription,
  - Check that Wednesday and Thursday are still checked
  - Renew subscription from 2017-10-16
  - Edit subscription,
  - Make sure that Wednesday and Thursday are still checked
    with corresponding serials

  - Do the same for a monthly and weekly subscription

Rebased - 2017-12-15 - Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Rebased - 2021-03-04 - Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 132413 [details] [review]
Bug 17656: (follow-up) Add missing filters

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 132415 [details] [review]
Bug 17656: [DO NOT PUSH] provide schema file

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 132416 [details] [review]
Bug 17656: QA warnings fixes

Signed-off-by: Amarangi  <amarangikarunathilaka@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 132417 [details] [review]
Bug 17656: use Koha::Subscription->startdate instead of Koha::Subscriptions

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 132418 [details] [review]
Bug 17656: More meaningful messages in prediction pattern table

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 132447 [details] [review]
Bug 17656: fix issue_number call in showprediction.pl

Created attachment 132448 [details] [review]
Bug 17656: (follow-up) Fix typo for QA

I've got an error trace on cgi-bin/koha/serials/subscription-renew.pl?mode=popup&subscriptionid=1 when I try to renew the subscription.
See the full error in text document attached.

Created attachment 132692 [details]
Error trace when renew a subscription

Created attachment 132779 [details] [review]
Bug 17656: (follow-up) Update DBIC + fix missing C4::Serials functions needed

I still have an error trace, this time when I save the subscription.

With the patch applies, the option to set 'regular irregularities' for daily (text + checkbox) are not displayed anymore. It didn't happen the first time I test the patch.

Up ^^