Bug 18745 - Serial claims: save supplier info on serial-level
Summary: Serial claims: save supplier info on serial-level
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on: 18782
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-07 15:29 UTC by Julian Maurice
Modified: 2023-09-01 20:53 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18745 - Serial claims: save supplier info on serial-level (11.94 KB, patch)
2017-06-07 15:31 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18745 - kohastructure.sql: Move aqbooksellers above serial (5.71 KB, patch)
2017-08-10 08:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18745: Serial claims: save supplier info on serial-level (11.93 KB, patch)
2019-02-18 12:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18745: kohastructure.sql: Move aqbooksellers above serial (5.74 KB, patch)
2019-02-18 12:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18745: Add foreign key constraint on subscription.aqbooksellerid (4.76 KB, patch)
2019-02-18 12:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18745: Serial claims: save supplier info on serial-level (11.93 KB, patch)
2019-03-27 17:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18745: kohastructure.sql: Move aqbooksellers above serial (5.74 KB, patch)
2019-03-27 17:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18745: Add foreign key constraint on subscription.aqbooksellerid (4.77 KB, patch)
2019-03-27 17:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18745: Serial claims: save supplier info on serial-level (35.04 KB, patch)
2021-09-17 08:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18745: Serial claims: save supplier info on serial-level (16.52 KB, patch)
2022-07-31 22:53 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
[DO NOT PUSH] Bug 18745: DBIC schema changes (5.26 KB, patch)
2022-07-31 22:53 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2017-06-07 15:29:58 UTC

    
Comment 1 Julian Maurice 2017-06-07 15:31:43 UTC
Created attachment 64083 [details] [review]
Bug 18745 - Serial claims: save supplier info on serial-level

This allows to change the subscription's supplier while keeping the info
of supplier for previous serials, which allows to claim late/missing
serials to the right supplier

Test plan:
1. Create 2 suppliers, S1 and S2
2. Create a new subscription with S1
3. Generate some late/missing serials
4. Change supplier to S2
5. Generate some more late/missing serials
6. Go to /cgi-bin/koha/serials/claims.pl and see you have late/missing
issues for both suppliers
7. Check that you have the expected issues for each supplier
8. prove t/db_dependent/Serials/Claims.t
Comment 2 Julian Maurice 2017-08-10 08:37:18 UTC
Created attachment 65791 [details] [review]
Bug 18745 - kohastructure.sql: Move aqbooksellers above serial

Otherwise we can't define a foreign key in serial that references
aqbooksellers
Comment 3 Alex Buckley 2017-10-04 02:39:55 UTC
Hi Julian

When performing step 2 (creating a new subscription for supplier S1) I found that a software error was thrown (see below). You will need to add the aqbooksellerid to the Koha::Schema::Result::Serial

Software error:

DBIx::Class::Row::store_column(): No such column 'aqbooksellerid' on Koha::Schema::Result::Serial at /home/vagrant/kohaclone/Koha/Object.pm line 72
Comment 4 Julian Maurice 2017-10-04 07:34:44 UTC
Hi Alex,

Something is missing in the test plan, you need to run the database update and update the DBIx::Class schema files first.

    perl installer/data/mysql/updatedatabase.pl
    perl misc/devel/update_dbix_class_files.pl --db_name DBNAME --db_user DBUSER --db_passwd DBPASS
Comment 5 Katrin Fischer 2017-10-07 17:43:47 UTC
Hi Julian,

I've run the database update, dbic, and restart_all in this sequence. The database updates look fine, the aqbooksellerid is populated in serial. 

But when I try to receive an issue, I have an error:

Internal server error

koha-plack-error:

Can't call method "title" on an undefined value at /home/vagrant/kohaclone/serials/serials-edit.pl line 424.
Comment 6 Victor Grousset/tuxayo 2019-01-25 11:26:31 UTC
Hi, when creating a serial without a vendor, there is an internal server error.

logs:

DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha`.`serial`, CONSTRAINT `serial_aqbooksellerid_fk` FOREIGN KEY (`aqbooksellerid`) REFERENCES `aqbooksellers` (`id`)) [for Statement "INSERT INTO `serial` ( `aqbooksellerid`, `biblionumber`, `planneddate`, `publisheddate`, `serialseq`, `serialseq_x`, `serialseq_y`, `serialseq_z`, `status`, `subscriptionid`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='0', 1="12345", 2='2019-01-01', 3='2019-01-01', 4='Vol 145, No 1,2019', 5='145', 6='1', 7='2019', 8=1, 9='123'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.
DBIx::Class::Storage::DBI::_dbh_execute(): Cannot add or update a child row: a foreign key constraint fails (`koha`.`serial`, CONSTRAINT `serial_aqbooksellerid_fk` FOREIGN KEY (`aqbooksellerid`) REFERENCES `aqbooksellers` (`id`)) at /home/koha/src/Koha/Object.pm line 121

It seems that the issue is that 0 is trying to get in "aqbooksellerid".

And there is no aqbooksellers.aqbooksellerid egal to 0
Comment 7 Julian Maurice 2019-02-18 12:50:35 UTC
Created attachment 85240 [details] [review]
Bug 18745: Serial claims: save supplier info on serial-level

This allows to change the subscription's supplier while keeping the info
of supplier for previous serials, which allows to claim late/missing
serials to the right supplier

Test plan:
1. Create 2 suppliers, S1 and S2
2. Create a new subscription with S1
3. Generate some late/missing serials
4. Change supplier to S2
5. Generate some more late/missing serials
6. Go to /cgi-bin/koha/serials/claims.pl and see you have late/missing
issues for both suppliers
7. Check that you have the expected issues for each supplier
8. prove t/db_dependent/Serials/Claims.t
Comment 8 Julian Maurice 2019-02-18 12:50:43 UTC
Created attachment 85241 [details] [review]
Bug 18745: kohastructure.sql: Move aqbooksellers above serial

Otherwise we can't define a foreign key in serial that references
aqbooksellers
Comment 9 Julian Maurice 2019-02-18 12:50:51 UTC
Created attachment 85242 [details] [review]
Bug 18745: Add foreign key constraint on subscription.aqbooksellerid
Comment 10 Victor Grousset/tuxayo 2019-02-18 13:41:45 UTC
So now aqbookseller is mandatory in the form right?
Comment 11 Katrin Fischer 2019-02-18 14:05:20 UTC
(In reply to Victor Grousset/tuxayo from comment #10)
> So now aqbookseller is mandatory in the form right?

I'd like to avoid this if possible. Right now there is only a warning and I think assigning a 'fake' vendor in some cases is not as nice as the current behaviour.
Comment 12 Victor Grousset/tuxayo 2019-02-18 14:46:56 UTC
Are there cases where a serial wouldn't always have a vendor? From a librarian POV.
Comment 13 Julian Maurice 2019-02-18 15:08:36 UTC
(In reply to Katrin Fischer from comment #11)
> (In reply to Victor Grousset/tuxayo from comment #10)
> > So now aqbookseller is mandatory in the form right?
> 
> I'd like to avoid this if possible. Right now there is only a warning and I
> think assigning a 'fake' vendor in some cases is not as nice as the current
> behaviour.

Bookseller is still optional, but now subscription.aqbooksellerid would be NULL instead of 0
Comment 14 Katrin Fischer 2019-02-18 16:04:12 UTC
(In reply to Victor Grousset/tuxayo from comment #12)
> Are there cases where a serial wouldn't always have a vendor? From a
> librarian POV.

Some don't use the claim feature or acq... and then the vendor might be left empty. Also maybe if you are tracking your own publications or loose leaf things? I am not sure, but I have been asked if the field can be left empty.
Comment 15 Julian Maurice 2019-03-27 17:10:44 UTC
Created attachment 87102 [details] [review]
Bug 18745: Serial claims: save supplier info on serial-level

This allows to change the subscription's supplier while keeping the info
of supplier for previous serials, which allows to claim late/missing
serials to the right supplier

Test plan:
1. Create 2 suppliers, S1 and S2
2. Create a new subscription with S1
3. Generate some late/missing serials
4. Change supplier to S2
5. Generate some more late/missing serials
6. Go to /cgi-bin/koha/serials/claims.pl and see you have late/missing
issues for both suppliers
7. Check that you have the expected issues for each supplier
8. prove t/db_dependent/Serials/Claims.t
Comment 16 Julian Maurice 2019-03-27 17:10:50 UTC
Created attachment 87103 [details] [review]
Bug 18745: kohastructure.sql: Move aqbooksellers above serial

Otherwise we can't define a foreign key in serial that references
aqbooksellers
Comment 17 Julian Maurice 2019-03-27 17:10:53 UTC
Created attachment 87104 [details] [review]
Bug 18745: Add foreign key constraint on subscription.aqbooksellerid
Comment 18 Laurence Rault 2019-10-02 15:33:56 UTC
Applying: Bug 18745: kohastructure.sql: Move aqbooksellers above serial
Applying: Bug 18745: Add foreign key constraint on subscription.aqbooksellerid
fatal: sha1 information is lacking or useless (C4/Serials.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 18745: Add foreign key constraint on subscription.aqbooksellerid
Comment 19 Julian Maurice 2021-09-17 08:03:32 UTC
Created attachment 124951 [details] [review]
Bug 18745: Serial claims: save supplier info on serial-level

This allows to change the subscription's supplier while keeping the info
of supplier for previous serials, which allows to claim late/missing
serials to the right supplier

Test plan:
1. Create 2 suppliers, S1 and S2
2. Create a new subscription with S1
3. Generate some late/missing serials
4. Change supplier to S2
5. Generate some more late/missing serials
6. Go to /cgi-bin/koha/serials/claims.pl and see you have late/missing
issues for both suppliers
7. Check that you have the expected issues for each supplier
8. prove t/db_dependent/Serials/Claims.t
Comment 20 Julian Maurice 2021-09-17 08:04:40 UTC
All patches rebased on master and squashed. Ready to be tested
Comment 21 Jonathan Druart 2021-12-15 09:41:52 UTC
error: sha1 information is lacking or useless (installer/data/mysql/kohastructure.sql).

You shouldn't need to reorder kohastructure.sql
Comment 22 Fridolin Somers 2022-07-29 08:17:14 UTC
(In reply to Jonathan Druart from comment #21)
> error: sha1 information is lacking or useless
> (installer/data/mysql/kohastructure.sql).
> 
> You shouldn't need to reorder kohastructure.sql

Indeed patch "kohastructure.sql: Move aqbooksellers above serial" is obsolete, aqbooksellers is above serial because tables are alphabetical now
Comment 23 Victor Grousset/tuxayo 2022-07-31 22:53:38 UTC
Created attachment 138423 [details] [review]
Bug 18745: Serial claims: save supplier info on serial-level

This allows to change the subscription's supplier while keeping the info
of supplier for previous serials, which allows to claim late/missing
serials to the right supplier

Test plan:
1. Create 2 suppliers, S1 and S2
2. Create a new subscription with S1
3. Generate some late/missing serials
4. Change supplier to S2
5. Generate some more late/missing serials
6. Go to /cgi-bin/koha/serials/claims.pl and see you have late/missing
issues for both suppliers
7. Check that you have the expected issues for each supplier
8. prove t/db_dependent/Serials/Claims.t

Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 24 Victor Grousset/tuxayo 2022-07-31 22:53:43 UTC
Created attachment 138424 [details] [review]
[DO NOT PUSH] Bug 18745: DBIC schema changes
Comment 25 Victor Grousset/tuxayo 2022-07-31 23:05:02 UTC
Happy birthday, here is a rebase 🎁 ^o^

I compared the old and new patch files to double check that I didn't change anything unknowingly.


There was some mess in the ticket. Some old patches weren't obsoleted and this was the actual reason the patch didn't apply at first.

Fixed missing comma in kohastructure.sql

Removed DBIC schema changes, regenerated then and put then in their own commit.
Comment 26 Victor Grousset/tuxayo 2022-07-31 23:09:36 UTC
(tests still pass)
Comment 27 Victor Grousset/tuxayo 2022-08-02 22:28:35 UTC
It seems there is an issue (but I'm not sure I'm using serials right)

> 3. Generate some late/missing serials

So I click "Receive" and replace "expected" by "late" or "missing" and then "save"
In my case I did one late and one missing.
Then changed the sub vendor to S2.
And did one late and one missing.

Now on claims for S1 I have one late, one missing, **one late** I had already changed to S2!

For S2 I have one missing, one expected. **But not one late**

Tried again and same result.

So the 1st after the transition (editing the sub to change vendor) is on the wrong vendor.

====

The tests pass.
the first "# 3 is late" looks like it should say "# 2 is late", should it?

Why the does the test expects
2 late issues for bookseller1
and
1 late issue for bookseller2

Shouldn't it be 1 and 2?
Or maybe I don't get how late serial work and the test is correct and the behavior of my manual testing is correct?
Comment 28 Fridolin Somers 2023-09-01 20:53:31 UTC
+            ADD CONSTRAINT `serial_aqbooksellerid_fk` FOREIGN KEY (`aqbooksellerid`) REFERENCES `aqbooksellers` (`id`)

Needs an explicit on delete/update cascade no ?


atomicupdate bug_18745.perl needs to be updated with current format.