Bug 29275 - Use the API to render checkout history for a biblio
Summary: Use the API to render checkout history for a biblio
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 29290 29523 29575
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-19 14:09 UTC by Liz Rea
Modified: 2024-03-27 14:30 UTC (History)
9 users (show)

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


Attachments
Bug 29275: Add $patron_to_html to render patron information in JS (2.38 KB, patch)
2021-10-20 21:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29275: Use the API to fetch checkouts information (15.86 KB, patch)
2021-10-20 21:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber (1.12 KB, patch)
2021-10-20 21:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29275: Add $patron_to_html to render patron information in JS (2.44 KB, patch)
2021-10-21 02:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 29275: Use the API to fetch checkouts information (15.92 KB, patch)
2021-10-21 02:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber (1.18 KB, patch)
2021-10-21 02:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 29275: Add $patron_to_html to render patron information in JS (2.44 KB, patch)
2021-11-16 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29275: Use the API to fetch checkouts information (13.16 KB, patch)
2021-11-16 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber (1.18 KB, patch)
2021-11-16 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29275: (follow-up) Split columns settings and handle upgrade (5.45 KB, patch)
2021-11-16 19:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29275: Add $patron_to_html to render patron information in JS (2.44 KB, patch)
2021-11-19 18:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29275: Use the API to fetch checkouts information (13.42 KB, patch)
2021-11-19 18:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber (1.18 KB, patch)
2021-11-19 18:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29275: (follow-up) Split columns settings and handle upgrade (5.45 KB, patch)
2021-11-19 18:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29275: Use the API to fetch checkouts information (13.48 KB, patch)
2021-11-25 11:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber (1.24 KB, patch)
2021-11-25 11:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29275: (follow-up) Split columns settings and handle upgrade (5.51 KB, patch)
2021-11-25 11:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29275: Use the API to fetch checkouts information (13.50 KB, patch)
2022-03-03 15:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber (1.24 KB, patch)
2022-03-03 15:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29275: (follow-up) Split columns settings and handle upgrade (5.51 KB, patch)
2022-03-03 15:38 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 Liz Rea 2021-10-19 14:09:35 UTC
Example:

We have a library with a biblio that has 39000ish issues in the history, this page cannot be loaded within the plack/apache timeouts, and users can DOS themselves trying to see it. 

This is admittedly a very extreme case, but even bibs with fewer issues in the history would probably benefit from this page having better performance.
Comment 1 Tomás Cohen Arazi 2021-10-20 21:36:04 UTC
Created attachment 126626 [details] [review]
Bug 29275: Add $patron_to_html to render patron information in JS

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-10-20 21:36:10 UTC
Created attachment 126627 [details] [review]
Bug 29275: Use the API to fetch checkouts information

This patch changes issuehistory.pl so it leverages on the API for
fetching checkouts information in the template.

Workflow changes: This development splits the 'current' checkouts and
the 'past' checkouts in two different tables.

Terminology could be revisited by native english speakers.

To test:
1. Have records with and without current/past checkouts
2. Notice how the Checkouts history tab presents the information on each
   case.
=> SUGGESTION: Keep each of them open on separate tabs for comparing
3. Apply this patchset and its dependencies
4. Restart plack, run:
   $ koha-plack --restart kohadev
5. Follow the same steps you followed for those open tabs, in new ones
=> SUCCESS: What you see makes sense, information is displayed as it
used to, with the different tables now.
6. Try searching on the datatables
=> SUCCESS: Search works correctly!
7. Revisit the new texts I added
=> SUGGESTION: If you have better wordings, submit a patch, or just
comment on the bug so I do it inline.
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2021-10-20 21:36:16 UTC
Created attachment 126628 [details] [review]
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Victor Grousset/tuxayo 2021-10-21 02:36:58 UTC
Created attachment 126634 [details] [review]
Bug 29275: Add $patron_to_html to render patron information in JS

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 5 Victor Grousset/tuxayo 2021-10-21 02:37:02 UTC
Created attachment 126635 [details] [review]
Bug 29275: Use the API to fetch checkouts information

This patch changes issuehistory.pl so it leverages on the API for
fetching checkouts information in the template.

Workflow changes: This development splits the 'current' checkouts and
the 'past' checkouts in two different tables.

Terminology could be revisited by native english speakers.

To test:
1. Have records with and without current/past checkouts
2. Notice how the Checkouts history tab presents the information on each
   case.
=> SUGGESTION: Keep each of them open on separate tabs for comparing
3. Apply this patchset and its dependencies
4. Restart plack, run:
   $ koha-plack --restart kohadev
5. Follow the same steps you followed for those open tabs, in new ones
=> SUCCESS: What you see makes sense, information is displayed as it
used to, with the different tables now.
6. Try searching on the datatables
=> SUCCESS: Search works correctly!
7. Revisit the new texts I added
=> SUGGESTION: If you have better wordings, submit a patch, or just
comment on the bug so I do it inline.
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2021-10-21 02:37:06 UTC
Created attachment 126636 [details] [review]
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Victor Grousset/tuxayo 2021-10-21 02:37:45 UTC
cases tested to get all the strings and behaviors I could thing of

- never checked out and no current checkout
- never checked out and a current checkout
- a past checkout and no current checkout
- more than 10 past checkout (for pagination) and a current checkout

strings make sense
Comment 8 Victor Grousset/tuxayo 2021-10-21 02:43:51 UTC
A diff between current_checkouts and old_checkouts functions show that they have a lot of duplication.
Is there a way to deduplicate without making something messy?
I wonder if there is a way to assign old_checkouts as a copy of current_checkouts and then re assign so it it's properties to add and change stuff.
The only difference is the URL param and a render function.

Also a call to encodeURIComponent() might be missing in current_checkouts
Comment 9 Tomás Cohen Arazi 2021-10-22 13:48:21 UTC
(In reply to Victor Grousset/tuxayo from comment #8)
> A diff between current_checkouts and old_checkouts functions show that they
> have a lot of duplication.
> Is there a way to deduplicate without making something messy?
> I wonder if there is a way to assign old_checkouts as a copy of
> current_checkouts and then re assign so it it's properties to add and change
> stuff.
> The only difference is the URL param and a render function.
> 
> Also a call to encodeURIComponent() might be missing in current_checkouts

Thanks! Fixing!
Comment 10 Tomás Cohen Arazi 2021-10-27 12:12:48 UTC
(In reply to Victor Grousset/tuxayo from comment #8)
> A diff between current_checkouts and old_checkouts functions show that they
> have a lot of duplication.
> Is there a way to deduplicate without making something messy?
> I wonder if there is a way to assign old_checkouts as a copy of
> current_checkouts and then re assign so it it's properties to add and change
> stuff.
> The only difference is the URL param and a render function.

I tried, but things get a bit harder to understand and I didn't feel like the trade-off justified a generalization.
Comment 11 Tomás Cohen Arazi 2021-10-27 12:14:10 UTC
I didn't submit the follow-up because the RM mentioned bug 28859, which is expected to be pushed first.
Comment 12 Victor Grousset/tuxayo 2021-10-31 00:37:33 UTC
And it has been pushed :D
Comment 13 Tomás Cohen Arazi 2021-11-16 19:11:10 UTC
Created attachment 127701 [details] [review]
Bug 29275: Add $patron_to_html to render patron information in JS

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 14 Tomás Cohen Arazi 2021-11-16 19:11:15 UTC
Created attachment 127702 [details] [review]
Bug 29275: Use the API to fetch checkouts information

This patch changes issuehistory.pl so it leverages on the API for
fetching checkouts information in the template.

Workflow changes: This development splits the 'current' checkouts and
the 'past' checkouts in two different tables.

Terminology could be revisited by native english speakers.

To test:
1. Have records with and without current/past checkouts
2. Notice how the Checkouts history tab presents the information on each
   case.
=> SUGGESTION: Keep each of them open on separate tabs for comparing
3. Apply this patchset and its dependencies
4. Restart plack, run:
   $ koha-plack --restart kohadev
5. Follow the same steps you followed for those open tabs, in new ones
=> SUCCESS: What you see makes sense, information is displayed as it
used to, with the different tables now.
6. Try searching on the datatables
=> SUCCESS: Search works correctly!
7. Revisit the new texts I added
=> SUGGESTION: If you have better wordings, submit a patch, or just
comment on the bug so I do it inline.
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 15 Tomás Cohen Arazi 2021-11-16 19:11:20 UTC
Created attachment 127703 [details] [review]
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 16 Tomás Cohen Arazi 2021-11-16 19:19:45 UTC
(In reply to Victor Grousset/tuxayo from comment #8)
> A diff between current_checkouts and old_checkouts functions show that they
> have a lot of duplication.
> Is there a way to deduplicate without making something messy?
> I wonder if there is a way to assign old_checkouts as a copy of
> current_checkouts and then re assign so it it's properties to add and change
> stuff.
> The only difference is the URL param and a render function.

Not just that. But I made a function to avoid duplication.

> Also a call to encodeURIComponent() might be missing in current_checkouts

Solved!
Comment 17 Tomás Cohen Arazi 2021-11-16 19:52:33 UTC
Created attachment 127704 [details] [review]
Bug 29275: (follow-up) Split columns settings and handle upgrade

This patch splits the table settings as we now have two tables. The
atomic update takes care of setting the right values (if present) on the
new tables entries.

To test:
1. Do not apply this patch
2. Set some values you will remember for the checkoutshistory-table
   table
3. Run:
   $ koha-mysql kohadev
   > SELECT * FROM columns_settings WHERE tablename='checkoutshistory-table';
=> SUCCESS: Koha currently works :-D
4. Apply this patch
5. Run:
   $ updatedatabase
6. Repeat 3
=> SUCCESS: Things have been deleted
7. Run:
   $ koha-mysql kohadev
   > SELECT * FROM columns_settings WHERE tablename LIKE '%_checkouts-table';
=> SUCCESS: old_checkouts-table and current_checkouts-table have the
right setting
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Martin Renvoize 2021-11-17 08:17:35 UTC
Sorry dude.. I love this improvement.. but I think we have a problem.  The new js equivalent to patron-name.inc is awesome and works great.. but it got me looking at what patron-name.inc does and threw me into the rabbit hole looking at how patrons get hidden from other branch staff in certain modes of operation.

I can't see any handling, either in the js function or in the API response builder, that would filter out patrons that the logged-in user should not be able to see details for.  I'm hopeful that I'm just missing something in the API layer as I think that's where it should sit personally.. we shouldn't expose the data at all if the user doesn't have permission to view it, rather than hide it at the view stage.  If that functionality is there, any chance you could point me to the unit tests for it?
Comment 19 Tomás Cohen Arazi 2021-11-19 18:39:54 UTC
Created attachment 127894 [details] [review]
Bug 29275: Add $patron_to_html to render patron information in JS

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 20 Tomás Cohen Arazi 2021-11-19 18:39:58 UTC
Created attachment 127895 [details] [review]
Bug 29275: Use the API to fetch checkouts information

This patch changes issuehistory.pl so it leverages on the API for
fetching checkouts information in the template.

Workflow changes: This development splits the 'current' checkouts and
the 'past' checkouts in two different tables.

Terminology could be revisited by native english speakers.

To test:
1. Have records with and without current/past checkouts
2. Notice how the Checkouts history tab presents the information on each
   case.
=> SUGGESTION: Keep each of them open on separate tabs for comparing
3. Apply this patchset and its dependencies
4. Restart plack, run:
   $ koha-plack --restart kohadev
5. Follow the same steps you followed for those open tabs, in new ones
=> SUCCESS: What you see makes sense, information is displayed as it
used to, with the different tables now.
6. Try searching on the datatables
=> SUCCESS: Search works correctly!
7. Revisit the new texts I added
=> SUGGESTION: If you have better wordings, submit a patch, or just
comment on the bug so I do it inline.
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 21 Tomás Cohen Arazi 2021-11-19 18:40:03 UTC
Created attachment 127896 [details] [review]
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 22 Tomás Cohen Arazi 2021-11-19 18:40:08 UTC
Created attachment 127897 [details] [review]
Bug 29275: (follow-up) Split columns settings and handle upgrade

This patch splits the table settings as we now have two tables. The
atomic update takes care of setting the right values (if present) on the
new tables entries.

To test:
1. Do not apply this patch
2. Set some values you will remember for the checkoutshistory-table
   table
3. Run:
   $ koha-mysql kohadev
   > SELECT * FROM columns_settings WHERE tablename='checkoutshistory-table';
=> SUCCESS: Koha currently works :-D
4. Apply this patch
5. Run:
   $ updatedatabase
6. Repeat 3
=> SUCCESS: Things have been deleted
7. Run:
   $ koha-mysql kohadev
   > SELECT * FROM columns_settings WHERE tablename LIKE '%_checkouts-table';
=> SUCCESS: old_checkouts-table and current_checkouts-table have the
right setting
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2021-11-19 18:44:59 UTC
(In reply to Martin Renvoize from comment #18)
> Sorry dude.. I love this improvement.. but I think we have a problem.  The
> new js equivalent to patron-name.inc is awesome and works great.. but it got
> me looking at what patron-name.inc does and threw me into the rabbit hole
> looking at how patrons get hidden from other branch staff in certain modes
> of operation.

Good catch! And this is bigger than this dev!

> I can't see any handling, either in the js function or in the API response
> builder, that would filter out patrons that the logged-in user should not be
> able to see details for.  I'm hopeful that I'm just missing something in the
> API layer as I think that's where it should sit personally.. we shouldn't
> expose the data at all if the user doesn't have permission to view it,
> rather than hide it at the view stage.  If that functionality is there, any
> chance you could point me to the unit tests for it?

I proposed a generic solution for checking 'allowed-to-embed' objects on bug 28523. It looks pretty solid to me and I made this bug dependent on it.

For the specific case of a 'null' patron on the response (which is now the case, correctly handled in the controller level, not just hidden) I decided to handle it locally (so not in js-patron-format.inc) like this:

if ( row.patron != null ) {
    return $patron_to_html( row.patron, { display_cardnumber: true, url: true } );
}
else {
    return _("A patron from library %s").format(escape_str(row.library.name));
}

it felt like there was no need for a generic message, as each scenario could require different wordings.

Please test!
Comment 24 Tomás Cohen Arazi 2021-11-19 19:48:42 UTC
(In reply to Victor Grousset/tuxayo from comment #12)
> And it has been pushed :D

Time to test :D
Comment 25 Martin Renvoize 2021-11-25 11:50:11 UTC
Created attachment 128001 [details] [review]
Bug 29275: Use the API to fetch checkouts information

This patch changes issuehistory.pl so it leverages on the API for
fetching checkouts information in the template.

Workflow changes: This development splits the 'current' checkouts and
the 'past' checkouts in two different tables.

Terminology could be revisited by native english speakers.

To test:
1. Have records with and without current/past checkouts
2. Notice how the Checkouts history tab presents the information on each
   case.
=> SUGGESTION: Keep each of them open on separate tabs for comparing
3. Apply this patchset and its dependencies
4. Restart plack, run:
   $ koha-plack --restart kohadev
5. Follow the same steps you followed for those open tabs, in new ones
=> SUCCESS: What you see makes sense, information is displayed as it
used to, with the different tables now.
6. Try searching on the datatables
=> SUCCESS: Search works correctly!
7. Revisit the new texts I added
=> SUGGESTION: If you have better wordings, submit a patch, or just
comment on the bug so I do it inline.
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2021-11-25 11:50:15 UTC
Created attachment 128002 [details] [review]
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2021-11-25 11:50:18 UTC
Created attachment 128003 [details] [review]
Bug 29275: (follow-up) Split columns settings and handle upgrade

This patch splits the table settings as we now have two tables. The
atomic update takes care of setting the right values (if present) on the
new tables entries.

To test:
1. Do not apply this patch
2. Set some values you will remember for the checkoutshistory-table
   table
3. Run:
   $ koha-mysql kohadev
   > SELECT * FROM columns_settings WHERE tablename='checkoutshistory-table';
=> SUCCESS: Koha currently works :-D
4. Apply this patch
5. Run:
   $ updatedatabase
6. Repeat 3
=> SUCCESS: Things have been deleted
7. Run:
   $ koha-mysql kohadev
   > SELECT * FROM columns_settings WHERE tablename LIKE '%_checkouts-table';
=> SUCCESS: old_checkouts-table and current_checkouts-table have the
right setting
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2021-11-25 11:54:05 UTC
OK, I split out the first patch and moved it to bug 29575 and set it as a dependency. This allows other bugs to utilise this great new function ;)

I've added my SO to the patches here and am just finishing up QAing the embed dependency.. will switch to PQA along with that one once I'm done.
Comment 29 Victor Grousset/tuxayo 2021-11-27 23:23:14 UTC
Good idea thanks! :D
Comment 30 Martin Renvoize 2022-03-03 15:38:11 UTC
Created attachment 131333 [details] [review]
Bug 29275: Use the API to fetch checkouts information

This patch changes issuehistory.pl so it leverages on the API for
fetching checkouts information in the template.

Workflow changes: This development splits the 'current' checkouts and
the 'past' checkouts in two different tables.

Terminology could be revisited by native english speakers.

To test:
1. Have records with and without current/past checkouts
2. Notice how the Checkouts history tab presents the information on each
   case.
=> SUGGESTION: Keep each of them open on separate tabs for comparing
3. Apply this patchset and its dependencies
4. Restart plack, run:
   $ koha-plack --restart kohadev
5. Follow the same steps you followed for those open tabs, in new ones
=> SUCCESS: What you see makes sense, information is displayed as it
used to, with the different tables now.
6. Try searching on the datatables
=> SUCCESS: Search works correctly!
7. Revisit the new texts I added
=> SUGGESTION: If you have better wordings, submit a patch, or just
comment on the bug so I do it inline.
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2022-03-03 15:38:15 UTC
Created attachment 131334 [details] [review]
Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2022-03-03 15:38:20 UTC
Created attachment 131335 [details] [review]
Bug 29275: (follow-up) Split columns settings and handle upgrade

This patch splits the table settings as we now have two tables. The
atomic update takes care of setting the right values (if present) on the
new tables entries.

To test:
1. Do not apply this patch
2. Set some values you will remember for the checkoutshistory-table
   table
3. Run:
   $ koha-mysql kohadev
   > SELECT * FROM columns_settings WHERE tablename='checkoutshistory-table';
=> SUCCESS: Koha currently works :-D
4. Apply this patch
5. Run:
   $ updatedatabase
6. Repeat 3
=> SUCCESS: Things have been deleted
7. Run:
   $ koha-mysql kohadev
   > SELECT * FROM columns_settings WHERE tablename LIKE '%_checkouts-table';
=> SUCCESS: old_checkouts-table and current_checkouts-table have the
right setting
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2022-03-03 15:39:08 UTC
A quick rebase, now moving on to testing.
Comment 34 Martin Renvoize 2022-03-03 15:50:22 UTC
OK, the rebase was trivial enough, but I think we need some more work in context of bug 29523.. and there's some weird things happening in the call to kohaTable.. for instance I'm not seeing the embeds get added to the query here in my testing.
Comment 35 Martin Renvoize 2022-06-28 16:14:34 UTC
OK, this needs another rebase and there's a bit more to it this time so I think I need to defer to you Tomas.
Comment 36 Martin Renvoize 2022-12-08 17:28:40 UTC
Eager to see this one move forward :)
Comment 37 Jonathan Druart 2023-05-23 13:09:50 UTC
+                            if ( row.patron != null ) {
 +                                return $patron_to_html( row.patron, { display_cardnumber: true, url: true } );
+                            }
+                            else {
+                                return _("A patron from library %s").format(escape_str(row.library.name));
                            }

This is not correct, is it?
row.library.name should be row.patron.library.name, but we don't have it.
Comment 38 Tomás Cohen Arazi 2023-05-23 13:25:30 UTC
(In reply to Jonathan Druart from comment #37)
> +                            if ( row.patron != null ) {
>  +                                return $patron_to_html( row.patron, {
> display_cardnumber: true, url: true } );
> +                            }
> +                            else {
> +                                return _("A patron from library
> %s").format(escape_str(row.library.name));
>                             }
> 
> This is not correct, is it?
> row.library.name should be row.patron.library.name, but we don't have it.

+Strings
Comment 39 Jonathan Druart 2023-05-23 14:09:55 UTC
(In reply to Tomás Cohen Arazi from comment #38)
> (In reply to Jonathan Druart from comment #37)
> > +                            if ( row.patron != null ) {
> >  +                                return $patron_to_html( row.patron, {
> > display_cardnumber: true, url: true } );
> > +                            }
> > +                            else {
> > +                                return _("A patron from library
> > %s").format(escape_str(row.library.name));
> >                             }
> > 
> > This is not correct, is it?
> > row.library.name should be row.patron.library.name, but we don't have it.
> 
> +Strings

lol yes if you want, but that was not my point :D
Comment 40 David Cook 2023-07-27 02:30:01 UTC
(In reply to Jonathan Druart from comment #37)
> +                            if ( row.patron != null ) {
>  +                                return $patron_to_html( row.patron, {
> display_cardnumber: true, url: true } );
> +                            }
> +                            else {
> +                                return _("A patron from library
> %s").format(escape_str(row.library.name));
>                             }
> 
> This is not correct, is it?
> row.library.name should be row.patron.library.name, but we don't have it.

It took me a minute, but I think I see what you're saying. "row.library.name" would be the name of the library where the checkout occurred - not the name of the library the patron is from. 

While it's possible that these two values could be the same, it's also possible that they could be different.

Wouldn't it make more sense to just change the text to something like: "Patron details suppressed", or "Unknown Patron", or even ""?
Comment 41 David Cook 2023-07-27 02:30:33 UTC
(In reply to David Cook from comment #40)
> Wouldn't it make more sense to just change the text to something like:
> "Patron details suppressed", or "Unknown Patron", or even ""?

"Patron details currently unavailable"
Comment 42 David Cook 2023-07-27 02:33:00 UTC
But I could also be misunderstanding how this is supposed to work...
Comment 43 Jonathan Druart 2023-07-27 07:59:32 UTC
(In reply to David Cook from comment #40)
> Wouldn't it make more sense to just change the text to something like:
> "Patron details suppressed", or "Unknown Patron", or even ""?

We could, but we remove information.
IIRC it was part of the specs (bug 18403).
Comment 44 Victor Grousset/tuxayo 2023-08-13 01:24:05 UTC
Gave a go at rebasing but can't arbitrate on a conflict about a change of HTML header level. (both in this patch and main/master)

Anyway, this doesn't seem to be needing a rebase right now but rather settling on what to do about comment 37 (continued in Comment 40 and Comment 43)
Comment 45 Martin Renvoize 2024-03-27 14:00:14 UTC
Still love to see this one move forward :S
Comment 46 Tomás Cohen Arazi 2024-03-27 14:30:34 UTC
Gonna do some QA work on bug 33568, and will align this patchset with what Jonathan has done there!