Bug 33871 - Add where parameter to sitemap.pl
Summary: Add where parameter to sitemap.pl
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 33870
Blocks:
  Show dependency treegraph
 
Reported: 2023-05-31 13:15 UTC by Marcel de Rooy
Modified: 2023-12-28 20:47 UTC (History)
3 users (show)

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


Attachments
Bug 33871: Use Koha object in Sitemapper, add optional filter (3.72 KB, patch)
2023-05-31 13:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33871: Test where parameter in Sitemapper.t (2.75 KB, patch)
2023-05-31 13:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33871: Add try catch in case where parameter is not valid SQL (1.77 KB, patch)
2023-07-14 10:36 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33871: Use Koha object in Sitemapper, add optional filter (3.74 KB, patch)
2023-07-17 09:54 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33871: Test where parameter in Sitemapper.t (2.76 KB, patch)
2023-07-17 09:54 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33871: Use Koha object in Sitemapper, add optional filter (3.81 KB, patch)
2023-09-01 12:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33871: Test where parameter in Sitemapper.t (3.03 KB, patch)
2023-09-01 12:10 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2023-05-31 13:15:31 UTC
Follow-up on 33870
Comment 1 Marcel de Rooy 2023-05-31 13:20:46 UTC
Created attachment 151879 [details] [review]
Bug 33871: Use Koha object in Sitemapper, add optional filter

Test plan:
Run misc/cronjobs/sitemap.pl -where "biblionumber>X" (replace X
by some clever value)
Verify results by browsing sitemap files.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2023-05-31 13:20:48 UTC
Created attachment 151880 [details] [review]
Bug 33871: Test where parameter in Sitemapper.t

This makes it possible to remove the ugly global deletes at the
start.

Test plan:
Run t/db_dependent/Sitemapper.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Pedro Amorim 2023-07-14 10:36:20 UTC
Created attachment 153458 [details] [review]
Bug 33871: Add try catch in case where parameter is not valid SQL

Hi Marcel is this try catch a good idea or irrelevant and unecessary?

Also, shoul we keep at least 1 test for ->run() (without params) in Sitemapper.t?
The where param in Sitemapper->run is optional, I think should still test for ->run()
Comment 4 Marcel de Rooy 2023-07-14 10:52:41 UTC
(In reply to Pedro Amorim from comment #3)
> Created attachment 153458 [details] [review] [review]
> Bug 33871: Add try catch in case where parameter is not valid SQL
> 
> Hi Marcel is this try catch a good idea or irrelevant and unecessary?
> 
> Also, shoul we keep at least 1 test for ->run() (without params) in
> Sitemapper.t?
> The where param in Sitemapper->run is optional, I think should still test
> for ->run()

Sounds good to me
Comment 5 Marcel de Rooy 2023-07-17 08:31:09 UTC
> (In reply to Pedro Amorim from comment #3)
> > Created attachment 153458 [details] [review] [review] [review]
> > Bug 33871: Add try catch in case where parameter is not valid SQL
> > 
> > Hi Marcel is this try catch a good idea or irrelevant and unecessary?

On second thought I would not add it. Note that we are just writing to a file here while reading db records. The DBIx exception is not really expected.

> > Also, shoul we keep at least 1 test for ->run() (without params) in
> > Sitemapper.t?
> > The where param in Sitemapper->run is optional, I think should still test
> > for ->run()

I will have a look.
Comment 6 Marcel de Rooy 2023-07-17 08:41:21 UTC
> > > Also, shoul we keep at least 1 test for ->run() (without params) in
> > > Sitemapper.t?
> > > The where param in Sitemapper->run is optional, I think should still test
> > > for ->run()

Strictly speaking perhaps yes. But looking at it, I think that we should be pragmatic. We would be testing if an empty where, i.e. a Koha objects search with empty parameters {} would return all records.
To make the test perform on a larger database without deleting all records, would again need a mock on ->process. Probably too much complexity for a very small gain to confirm something that we know and it is tested elsewhere too..
Comment 7 Pedro Amorim 2023-07-17 09:54:23 UTC
Created attachment 153534 [details] [review]
Bug 33871: Use Koha object in Sitemapper, add optional filter

Test plan:
Run misc/cronjobs/sitemap.pl -where "biblionumber>X" (replace X
by some clever value)
Verify results by browsing sitemap files.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 8 Pedro Amorim 2023-07-17 09:54:26 UTC
Created attachment 153535 [details] [review]
Bug 33871: Test where parameter in Sitemapper.t

This makes it possible to remove the ugly global deletes at the
start.

Test plan:
Run t/db_dependent/Sitemapper.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 9 Marcel de Rooy 2023-07-17 09:55:18 UTC
(In reply to Pedro Amorim from comment #8)
> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Thx !
Comment 10 Jonathan Druart 2023-09-01 12:10:39 UTC
Created attachment 155107 [details] [review]
Bug 33871: Use Koha object in Sitemapper, add optional filter

Test plan:
Run misc/cronjobs/sitemap.pl -where "biblionumber>X" (replace X
by some clever value)
Verify results by browsing sitemap files.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2023-09-01 12:10:41 UTC
Created attachment 155108 [details] [review]
Bug 33871: Test where parameter in Sitemapper.t

This makes it possible to remove the ugly global deletes at the
start.

Test plan:
Run t/db_dependent/Sitemapper.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Tomás Cohen Arazi 2023-09-01 14:24:25 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 13 Fridolin Somers 2023-09-08 05:53:13 UTC
Not backported to 23.05.x