Bug 33970 - We need a "backend" column in "illrequestattributes" table
Summary: We need a "backend" column in "illrequestattributes" table
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Pedro Amorim
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 20640
Blocks:
  Show dependency treegraph
 
Reported: 2023-06-09 12:45 UTC by Pedro Amorim
Modified: 2023-10-26 08:30 UTC (History)
9 users (show)

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


Attachments
Bug 33970: update DB (1.38 KB, patch)
2023-06-09 14:16 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33970: atomicupdate file (2.69 KB, patch)
2023-06-09 14:16 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33970: [DONT PUSH] dbic file (1.71 KB, patch)
2023-06-09 14:16 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33970: update DB (1.44 KB, patch)
2023-07-10 09:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33970: atomicupdate file (2.74 KB, patch)
2023-07-10 09:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33970: [DONT PUSH] dbic file (1.76 KB, patch)
2023-07-10 09:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33970: Fix tests (1.36 KB, patch)
2023-08-29 15:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33970: Update DB (1.50 KB, patch)
2023-10-19 13:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33970: Atomicupdate file (2.71 KB, patch)
2023-10-19 13:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33970: [DONT PUSH] dbic file (1.82 KB, patch)
2023-10-19 13:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33970: Fix tests (2.43 KB, patch)
2023-10-19 13:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33970: Compatability schim (1.31 KB, patch)
2023-10-19 13:40 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-06-09 12:45:15 UTC

    
Comment 1 Pedro Amorim 2023-06-09 14:16:05 UTC
Created attachment 152250 [details] [review]
Bug 33970: update DB
Comment 2 Pedro Amorim 2023-06-09 14:16:07 UTC
Created attachment 152251 [details] [review]
Bug 33970: atomicupdate file
Comment 3 Pedro Amorim 2023-06-09 14:16:09 UTC
Created attachment 152252 [details] [review]
Bug 33970: [DONT PUSH] dbic file
Comment 4 Pedro Amorim 2023-06-09 14:17:50 UTC
Original issue is mentioned in bug 20640. Do a search for "Duplicate ID"

Don't apply patches. ILL set-up, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev-plus.sh)

The above will fetch the latest of FreeForm and BLDSS, that are already compatible with this.

Test plan:
- Create a new BLDSS request, enter "9780822370451" on "Keywords" input field
- Put "42" on cardnumber and pick a branch, submit
- Next screen, on the far right of the table, click the "Request <BLLID>" link
- Next screen, click the "Request" bottom button
- Next screen, click the "Switch provider" button and pick "FreeForm"
- Verify you have a "Duplicate ID" error, the logs say
- Duplicate entry '2-author' for key 'PRIMARY'

This is because both backends have a "author" illrequestattribute field.
And switching provider maintains the same ILL request (thus illrequest_id).
It's trying to add another row with composite key "1-author" in the illrequestattributes table.

Apply patches, and either reset_all or updatedatabase.pl
Do the test plan again, verify it now succeeds.

This should also cover the use-case where different backends have different meaning for equal attribute keys.
Comment 5 Pedro Amorim 2023-06-09 14:21:11 UTC
A possible interesting follow-up would be to create a bottom section in the "Manage request" page titled "Details from previous backend(s)", where we'd list all illrequestattributes from backends different than the current one.

Under the existing "Details from supplier (supplier name)" section, we'd update this list to just show the illrequestattributes from the current backend.
Comment 6 Pedro Amorim 2023-07-10 09:30:11 UTC
Created attachment 153263 [details] [review]
Bug 33970: update DB

Signed-off-by: Chris Walton <C.J.Walton@lboro.ac.uk>
Comment 7 Pedro Amorim 2023-07-10 09:30:14 UTC
Created attachment 153264 [details] [review]
Bug 33970: atomicupdate file

Signed-off-by: Chris Walton <C.J.Walton@lboro.ac.uk>
Comment 8 Pedro Amorim 2023-07-10 09:30:16 UTC
Created attachment 153265 [details] [review]
Bug 33970: [DONT PUSH] dbic file

Signed-off-by: Chris Walton <C.J.Walton@lboro.ac.uk>
Comment 9 Pedro Amorim 2023-08-04 09:42:41 UTC
(In reply to Pedro Amorim from comment #5)
> A possible interesting follow-up would be to create a bottom section in the
> "Manage request" page titled "Details from previous backend(s)", where we'd
> list all illrequestattributes from backends different than the current one.
> 
> Under the existing "Details from supplier (supplier name)" section, we'd
> update this list to just show the illrequestattributes from the current
> backend.

Scratch this. This list is provided by the backend's metadata method, hence must be solved on the backend, not core.
Comment 10 Pedro Amorim 2023-08-29 15:18:24 UTC
Created attachment 154903 [details] [review]
Bug 33970: Fix tests
Comment 11 Martin Renvoize 2023-10-19 13:40:16 UTC
Created attachment 157433 [details] [review]
Bug 33970: Update DB

Signed-off-by: Chris Walton <C.J.Walton@lboro.ac.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2023-10-19 13:40:19 UTC
Created attachment 157434 [details] [review]
Bug 33970: Atomicupdate file

Signed-off-by: Chris Walton <C.J.Walton@lboro.ac.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2023-10-19 13:40:21 UTC
Created attachment 157435 [details] [review]
Bug 33970: [DONT PUSH] dbic file

Signed-off-by: Chris Walton <C.J.Walton@lboro.ac.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2023-10-19 13:40:24 UTC
Created attachment 157436 [details] [review]
Bug 33970: Fix tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2023-10-19 13:40:27 UTC
Created attachment 157437 [details] [review]
Bug 33970: Compatability schim

This adds a compatability option to allow older backends not passing
their backend code through to the attributes on store.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2023-10-19 13:42:21 UTC
I've added a schim that I think should help with backward compatibility for backends.

Generally I think this is a really good move and shouldn't cause regressions.

I'm going to PQA (scripts are happy etc).. but leave to the RM to have a look.. the code is simple but the consequences could be interesting.
Comment 17 Tomás Cohen Arazi 2023-10-25 14:49:05 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 18 Fridolin Somers 2023-10-26 08:30:08 UTC
Enhancement not pushed to 23.05.x