Bug 30057

Summary: Move Virtualshelves exceptions to their own file
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: david, jonathan.druart, lucas, m.de.rooy, martin.renvoize, nick, victor
Version: unspecifiedKeywords: rel_22_11_candidate
Hardware: All   
OS: All   
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
Circulation function:
Bug Depends on: 29857    
Bug Blocks:    
Attachments: Bug 30057: Move virtualshelf-related exceptions to a separate library
Bug 30057: Move virtualshelf-related exceptions to a separate library
Bug 30057: Move virtualshelf-related exceptions to a separate library

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