Bug 17355 - Authorised value categories cannot be deleted
Summary: Authorised value categories cannot be deleted
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
: 18813 25275 (view as bug list)
Depends on: 17216
Blocks: 25651
  Show dependency treegraph
 
Reported: 2016-09-27 05:42 UTC by Katrin Fischer
Modified: 2020-06-05 16:25 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 17355 - Allow deletion of authorised value categories (4.29 KB, patch)
2017-06-08 13:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17355 - Allow deletion of authorised value categories (4.35 KB, patch)
2017-08-10 02:21 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 17355 - Allow deletion of authorised value categories (4.75 KB, patch)
2017-08-11 02:42 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 17355 - fixing typo in success message (1.08 KB, patch)
2017-08-11 02:46 UTC, Liz Rea
Details | Diff | Splinter Review
[SIGNED OFF] Bug 17355 - Allow deletion of authorised value categories (4.82 KB, patch)
2017-08-15 15:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 17355 - fixing typo in success message (1.15 KB, patch)
2017-08-15 15:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17355 - Follow-up - Fix page structure and alert on delete (2.13 KB, patch)
2017-08-15 15:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17355: DB changes (1.99 KB, patch)
2020-04-28 10:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17355: DBIC changes (1.40 KB, patch)
2020-04-28 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17355: DBIC changes - boolean flag (915 bytes, patch)
2020-04-28 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17355: Override delete methods (3.68 KB, patch)
2020-04-28 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17355: DB changes - set is_system for categories (3.75 KB, patch)
2020-04-28 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17355: Add the ability to remove authorised value categories (8.10 KB, patch)
2020-04-28 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17355: DB changes (2.04 KB, patch)
2020-05-01 14:22 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 17355: DBIC changes (1.46 KB, patch)
2020-05-01 14:22 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 17355: DBIC changes - boolean flag (973 bytes, patch)
2020-05-01 14:22 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 17355: Override delete methods (3.74 KB, patch)
2020-05-01 14:22 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 17355: DB changes - set is_system for categories (3.81 KB, patch)
2020-05-01 14:22 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 17355: Add the ability to remove authorised value categories (8.42 KB, patch)
2020-05-01 14:22 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 17355: DB changes (2.10 KB, patch)
2020-05-01 15:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17355: DBIC changes (1.51 KB, patch)
2020-05-01 15:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17355: DBIC changes - boolean flag (1.01 KB, patch)
2020-05-01 15:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17355: Override delete methods (3.79 KB, patch)
2020-05-01 15:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17355: DB changes - set is_system for categories (3.88 KB, patch)
2020-05-01 15:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17355: Add the ability to remove authorised value categories (8.49 KB, patch)
2020-05-01 15:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17355: (QA follow-up) Add description to updatedatabase entry (936 bytes, patch)
2020-05-01 15:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17355: Fix failing tests (4.47 KB, patch)
2020-05-05 08:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17355: (QA follow-up) Fix used package name (1.13 KB, patch)
2020-05-07 22:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2016-09-27 05:42:14 UTC
This will be a follow-up to bug 17216 that adds a separate table for authorised value categories. Now that we store them, we need a way to delete accidentally added categories easily.

Jonathan wrote about this:

- There is no way to delete a category (TODO). But to do so it would be
  good to have a authorised_value_categories.is_internal field to mark
  some categories as "cannot be deleted".
Comment 1 Nick Clemens 2017-06-08 13:18:48 UTC
Created attachment 64102 [details] [review]
Bug 17355 - Allow deletion of authorised value categories

This is jsut a ptach I was toying with - as Jonathan mentions we need a
way to prevent deletion of internal categories, I just listed them here
(we would list more)

Partial patch, so would need work but poking this bug as reminder :-)
Comment 2 Barton Chittenden 2017-06-15 16:03:27 UTC
*** Bug 18813 has been marked as a duplicate of this bug. ***
Comment 3 Liz Rea 2017-08-10 02:21:40 UTC
Created attachment 65782 [details] [review]
Bug 17355 - Allow deletion of authorised value categories

This is jsut a ptach I was toying with - as Jonathan mentions we need a
way to prevent deletion of internal categories, I just listed them here
(we would list more)

Partial patch, so would need work but poking this bug as reminder :-)

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
This is a good start
Comment 4 Liz Rea 2017-08-11 02:42:26 UTC
Created attachment 65869 [details] [review]
Bug 17355 - Allow deletion of authorised value categories

Note that this does not capture all internal fields, and isn't a database solution to the problem of marking internal fields.

To test:

Add an empty authorised value category, delete it. Make sure it stays deleted.

Try to delete an empty system authorised value, like ccode. You should not see the button.
Comment 5 Liz Rea 2017-08-11 02:46:17 UTC
Created attachment 65870 [details] [review]
Bug 17355 - fixing typo in success message
Comment 6 Katrin Fischer 2017-08-15 15:29:31 UTC
Should this be 'Needs Sign-off'?
Comment 7 Katrin Fischer 2017-08-15 15:44:33 UTC
Created attachment 66022 [details] [review]
[SIGNED OFF] Bug 17355 - Allow deletion of authorised value categories

Note that this does not capture all internal fields, and isn't a database solution to the problem of marking internal fields.

To test:

Add an empty authorised value category, delete it. Make sure it stays deleted.

Try to delete an empty system authorised value, like ccode. You should not see the button.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Katrin Fischer 2017-08-15 15:44:36 UTC
Created attachment 66023 [details] [review]
[SIGNED OFF] Bug 17355 - fixing typo in success message

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2017-08-15 15:44:40 UTC
Created attachment 66024 [details] [review]
Bug 17355 - Follow-up - Fix page structure and alert on delete

- Fixes page structure in 2 spots.
  Without the patch the second problem was manifesting in a
  display problem with the right side tool navigation.
- Adds a specific 'Are you sure you want to...' question
  instead of reusing the one for authorised values also for
  categories.
Comment 10 Jonathan Druart 2017-08-25 15:51:30 UTC
Do not you think that my suggestion to add a new "is_internal" (or whatever) new DB field could be better?
Comment 11 Katrin Fischer 2017-08-26 10:27:48 UTC
I just noticed that +        LOC => 1, is doubled up in the patch.
I think the idea of a database field is good, but maybe this could be done in a separate patch? We filed the bug a year ago :)
Comment 12 Julian Maurice 2017-09-01 12:01:18 UTC
QA comment:
1. It is possible to delete protected categories by changing the URL, e.g. /cgi-bin/koha/admin/authorised_values.pl?op=del_category&category=Bsort1
The deletion must be prevented on the server side.
2. The 'Delete category' button is inside the message box for "There are no authorised values ...". Why not in the toolbar like the other buttons ?
3. After a successful deletion, there is no redirect, so if we refresh the page the deletion is tried again and we end up with the following error message: "error_on_delete_cat". This message should be more human-friendly and IMO there should be a redirect after the deletion.
4. As comment 11 says, LOC appears twice in the hash, as well as LOST. These duplicates must be removed.
Comment 13 Katrin Fischer 2018-03-20 05:34:16 UTC
Can we revive this?
Comment 14 David Cook 2019-01-21 05:11:17 UTC
(In reply to Katrin Fischer from comment #13)
> Can we revive this?

I think that sounds like a good idea. I just had someone ask me to delete a category for them that they added by accident. It seemed unfortunate that they couldn't do it themself. 

I was thinking "user_defined" might be a better column for the database than "is_internal". Anything without that flag would be considered to be an internal/system value. 

I think this would future-proof us against new internal/system authorised values categories. For instance, in the existing patches, there are lots of categories missing which show up in the SQL packaged with the Koha installer. I think we only really want to worry about deleting categories that have been added by users, so we could just set that flag when users add via the Web UI.
Comment 15 Agnes Rivers-Moore 2020-02-05 19:41:52 UTC
I had to get help deleting a couple of accidental categories. Anyone still interested in finishing this up?
Comment 16 Katrin Fischer 2020-04-24 20:03:32 UTC
*** Bug 25275 has been marked as a duplicate of this bug. ***
Comment 17 Jonathan Druart 2020-04-28 10:07:57 UTC
Created attachment 103833 [details] [review]
Bug 17355: DB changes

This patch adds a new authorised_value_category.is_system boolean/tinyint(1)
DB field.

Name 'is_system' is coming from existing tables account_credit_types and
account_debit_types and should not be discussed here (if you disagree
fill a new bug report and deal with naming issue separately)
Comment 18 Jonathan Druart 2020-04-28 10:08:01 UTC
Created attachment 103834 [details] [review]
Bug 17355: DBIC changes
Comment 19 Jonathan Druart 2020-04-28 10:08:05 UTC
Created attachment 103835 [details] [review]
Bug 17355: DBIC changes - boolean flag
Comment 20 Jonathan Druart 2020-04-28 10:08:09 UTC
Created attachment 103836 [details] [review]
Bug 17355: Override delete methods
Comment 21 Jonathan Druart 2020-04-28 10:08:13 UTC
Created attachment 103837 [details] [review]
Bug 17355: DB changes - set is_system for categories
Comment 22 Jonathan Druart 2020-04-28 10:08:17 UTC
Created attachment 103838 [details] [review]
Bug 17355: Add the ability to remove authorised value categories

This new patch set is adding a new DB field is_system to distinguish
internal categories.

The list of internal categories is in the patch "DB changes - set
is_system for categories" and can be discussed/extended.

Test plan:
1.
- Create a new AV category
- Create an authorised value
- List the AVs for this category
- Remove all the AVs for this category
=> You see 2 buttons "Add a new authorised value" and "Delete category
'XXX'"
- Click the delete button
=> You get a popup
- Accept
=> The category is deleted

2.
- List the AV for Asort1 (internal category)
/cgi-bin/koha/admin/authorised_values.pl?searchfield=Asort1
- Delete the AVs for this category (if there is any)
=> You do not see the "Delete category" button

3.
- Create a new AV category 'foo'
- Create an authorised value
- Hit /admin/authorised_values.pl?op=delete_category&categor_name=foo
=> You get an error message (the category cannot be deleted if it has AV
linked)

4.
- Hit /admin/authorised_values.pl?op=delete_category&categor_name=Asort1
=> You get an error message (the category Asort1 cannot be deleted, it's an
internal category.
Comment 23 Victor Grousset/tuxayo 2020-05-01 14:11:31 UTC
Finding for QA:

> 3.
> - Create a new AV category 'foo'
> - Create an authorised value
> - Hit /admin/authorised_values.pl?op=delete_category&categor_name=foo
> => You get an error message (the category cannot be deleted if it has AV
> linked)

Message is « An error occurred when deleting this authorized value category. Check the logs. »

Plack error logs don't show anything.
Access logs show a 200 HTTP code.
Comment 24 Victor Grousset/tuxayo 2020-05-01 14:13:01 UTC
> 4.
> - Hit /admin/authorised_values.pl?op=delete_category&categor_name=Asort1
> => You get an error message (the category Asort1 cannot be deleted, it's an
> internal category.

Same.
Plack error logs don't show anything.
Access logs show a 200 HTTP code.

No issue for the signing off IMO
Comment 25 Victor Grousset/tuxayo 2020-05-01 14:22:00 UTC
Created attachment 104099 [details] [review]
Bug 17355: DB changes

This patch adds a new authorised_value_category.is_system boolean/tinyint(1)
DB field.

Name 'is_system' is coming from existing tables account_credit_types and
account_debit_types and should not be discussed here (if you disagree
fill a new bug report and deal with naming issue separately)

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 26 Victor Grousset/tuxayo 2020-05-01 14:22:04 UTC
Created attachment 104100 [details] [review]
Bug 17355: DBIC changes

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 27 Victor Grousset/tuxayo 2020-05-01 14:22:08 UTC
Created attachment 104101 [details] [review]
Bug 17355: DBIC changes - boolean flag

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 28 Victor Grousset/tuxayo 2020-05-01 14:22:13 UTC
Created attachment 104102 [details] [review]
Bug 17355: Override delete methods

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 29 Victor Grousset/tuxayo 2020-05-01 14:22:17 UTC
Created attachment 104103 [details] [review]
Bug 17355: DB changes - set is_system for categories

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 30 Victor Grousset/tuxayo 2020-05-01 14:22:22 UTC
Created attachment 104104 [details] [review]
Bug 17355: Add the ability to remove authorised value categories

This new patch set is adding a new DB field is_system to distinguish
internal categories.

The list of internal categories is in the patch "DB changes - set
is_system for categories" and can be discussed/extended.

Test plan:
0.
- Apply patch
- updatedabase
1.
- Create a new AV category
- Create an authorised value
- List the AVs for this category
- Remove all the AVs for this category
=> You see 2 buttons "Add a new authorised value" and "Delete category
'XXX'"
- Click the delete button
=> You get a popup
- Accept
=> The category is deleted

2.
- List the AV for Asort1 (internal category)
/cgi-bin/koha/admin/authorised_values.pl?searchfield=Asort1
- Delete the AVs for this category (if there is any)
=> You do not see the "Delete category" button

3.
- Create a new AV category 'foo'
- Create an authorised value
- Hit /admin/authorised_values.pl?op=delete_category&categor_name=foo
=> You get an error message (expected because the category cannot be
deleted if it has AV linked)
=> message text:
=> An error occurred when deleting this authorized value category. Check the logs.

4.
- Hit /admin/authorised_values.pl?op=delete_category&categor_name=Asort1
=> You get an error message (expected because the category Asort1 cannot
be deleted, it's an internal category.
=> message text:
=> An error occurred when deleting this authorized value category. Check the logs.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 31 Victor Grousset/tuxayo 2020-05-01 14:23:07 UTC
The following has been adding in the test plan:

0.
- Apply patch
- updatedabase


Tried to clarify test plan about the error messages. Don't expect them to tell the cause. QA: which is fine IMHO, because the messages are there in case of direct access with by URL.


Detailed parts:
in 3:
=> You get an error message (expected because the category cannot be
deleted if it has AV linked)
=> message text:
=> An error occurred when deleting this authorized value category. Check the logs.


in 4:
=> You get an error message (expected because the category Asort1 cannot
be deleted, it's an internal category.
=> message text:
=> An error occurred when deleting this authorized value category. Check the logs.
Comment 32 Katrin Fischer 2020-05-01 15:51:55 UTC
Created attachment 104117 [details] [review]
Bug 17355: DB changes

This patch adds a new authorised_value_category.is_system boolean/tinyint(1)
DB field.

Name 'is_system' is coming from existing tables account_credit_types and
account_debit_types and should not be discussed here (if you disagree
fill a new bug report and deal with naming issue separately)

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Katrin Fischer 2020-05-01 15:52:00 UTC
Created attachment 104118 [details] [review]
Bug 17355: DBIC changes

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Katrin Fischer 2020-05-01 15:52:05 UTC
Created attachment 104119 [details] [review]
Bug 17355: DBIC changes - boolean flag

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2020-05-01 15:52:10 UTC
Created attachment 104120 [details] [review]
Bug 17355: Override delete methods

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 36 Katrin Fischer 2020-05-01 15:52:15 UTC
Created attachment 104121 [details] [review]
Bug 17355: DB changes - set is_system for categories

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 37 Katrin Fischer 2020-05-01 15:52:20 UTC
Created attachment 104122 [details] [review]
Bug 17355: Add the ability to remove authorised value categories

This new patch set is adding a new DB field is_system to distinguish
internal categories.

The list of internal categories is in the patch "DB changes - set
is_system for categories" and can be discussed/extended.

Test plan:
0.
- Apply patch
- updatedabase
1.
- Create a new AV category
- Create an authorised value
- List the AVs for this category
- Remove all the AVs for this category
=> You see 2 buttons "Add a new authorised value" and "Delete category
'XXX'"
- Click the delete button
=> You get a popup
- Accept
=> The category is deleted

2.
- List the AV for Asort1 (internal category)
/cgi-bin/koha/admin/authorised_values.pl?searchfield=Asort1
- Delete the AVs for this category (if there is any)
=> You do not see the "Delete category" button

3.
- Create a new AV category 'foo'
- Create an authorised value
- Hit /admin/authorised_values.pl?op=delete_category&categor_name=foo
=> You get an error message (expected because the category cannot be
deleted if it has AV linked)
=> message text:
=> An error occurred when deleting this authorized value category. Check the logs.

4.
- Hit /admin/authorised_values.pl?op=delete_category&categor_name=Asort1
=> You get an error message (expected because the category Asort1 cannot
be deleted, it's an internal category.
=> message text:
=> An error occurred when deleting this authorized value category. Check the logs.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 38 Katrin Fischer 2020-05-01 15:52:25 UTC
Created attachment 104123 [details] [review]
Bug 17355: (QA follow-up) Add description to updatedatabase entry

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 39 Katrin Fischer 2020-05-01 16:24:30 UTC
This will make support staff's lives much easier. 

2 comments:
- I think the delete would be nice to have in the menu on the categories table as well (could be done laer9
- I wonder if 'is_internal' would work a bit better than 'is_system'
Comment 40 Martin Renvoize 2020-05-04 07:42:00 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 41 Jonathan Druart 2020-05-05 08:45:56 UTC
Created attachment 104352 [details] [review]
Bug 17355: Fix failing tests

This patch makes the tests pass without deleting mandatory AV cats
Comment 42 Tomás Cohen Arazi 2020-05-07 22:11:23 UTC
Created attachment 104539 [details] [review]
Bug 17355: (QA follow-up) Fix used package name

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 43 Joy Nelson 2020-05-08 23:14:26 UTC
not backported to 19.11