Bug 30057 - Move Virtualshelves exceptions to their own file
Summary: Move Virtualshelves exceptions to their own file
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords: rel_22_11_candidate
Depends on: 29857
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-09 12:46 UTC by Tomás Cohen Arazi
Modified: 2022-07-12 16:18 UTC (History)
7 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:
22.11.00, 22.05.03


Attachments
Bug 30057: Move virtualshelf-related exceptions to a separate library (10.66 KB, patch)
2022-02-09 13:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30057: Move virtualshelf-related exceptions to a separate library (10.71 KB, patch)
2022-04-19 19:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 30057: Move virtualshelf-related exceptions to a separate library (10.81 KB, patch)
2022-04-26 13:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2022-02-09 12:46:02 UTC
They are wrongly placed right now.
Comment 1 Tomás Cohen Arazi 2022-02-09 13:07:27 UTC
Created attachment 130359 [details] [review]
Bug 30057: Move virtualshelf-related exceptions to a separate library

This patch moves the not-generic virtualshelf-related exceptions to
their own file. Callers are adjusted as required.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Virtualshelves.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass
4. Check virtualshelves work as expected
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Jonathan Druart 2022-02-15 08:08:25 UTC
Is Koha::Exceptions::Virtualshelves::DuplicateObject a Koha::Exceptions::Virtualshelf or a Koha::Exceptions::DuplicateObject?

Shouldn't we actually have a mechanism to inherit from all the exceptions defined in Koha::Exception to not need to redefine them in each subclass?
Comment 3 Tomás Cohen Arazi 2022-02-16 17:54:36 UTC
(In reply to Jonathan Druart from comment #2)
> Is Koha::Exceptions::Virtualshelves::DuplicateObject a
> Koha::Exceptions::Virtualshelf or a Koha::Exceptions::DuplicateObject?

I looked at the code... it is basically a check on the name that's been typed. Which is not a DB constraint that would raise a Koha::Exceptions::Object::DuplicateID

It could be Koha::Exceptions::BadParameter too. DuplicatObject doesn't seem to allow a parameter to notify which attribute is guilty for the failure.

> Shouldn't we actually have a mechanism to inherit from all the exceptions
> defined in Koha::Exception to not need to redefine them in each subclass?

I think that's more aligned with Marcel's proposal, but going even further with no obvious big gain. Your proposal and Marcel's make me think we could check if there's another exceptions library that suits our needs better, as we will end-up doing it all ourselves.
Comment 4 Tomás Cohen Arazi 2022-04-04 22:14:29 UTC
Can we move this forward to be closer to finish this cleaning stage?
Comment 5 David Nind 2022-04-19 19:32:46 UTC
Created attachment 133414 [details] [review]
Bug 30057: Move virtualshelf-related exceptions to a separate library

This patch moves the not-generic virtualshelf-related exceptions to
their own file. Callers are adjusted as required.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Virtualshelves.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass
4. Check virtualshelves work as expected
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2022-04-19 19:35:43 UTC
Testing notes:

1. Tests pass before and after patch applied.

2. Ran through basic processes for creating and deleting a list, and adding items to a list, removing items, downloading, sending, etc. Tested in the staff interface and OPAC. Everything worked as expected.
Comment 7 Marcel de Rooy 2022-04-26 13:03:25 UTC
Created attachment 133882 [details] [review]
Bug 30057: Move virtualshelf-related exceptions to a separate library

This patch moves the not-generic virtualshelf-related exceptions to
their own file. Callers are adjusted as required.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Virtualshelves.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass
4. Check virtualshelves work as expected
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Tomás Cohen Arazi 2022-06-06 17:33:15 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 9 Lucas Gass 2022-07-12 16:18:42 UTC
Backported to 22.05.x for 22.05.03