Description
Katrin Fischer
2016-09-27 05:42:14 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 :-) *** Bug 18813 has been marked as a duplicate of this bug. *** 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 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. Created attachment 65870 [details] [review] Bug 17355 - fixing typo in success message Should this be 'Needs Sign-off'? 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> Created attachment 66023 [details] [review] [SIGNED OFF] Bug 17355 - fixing typo in success message Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> 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. Do not you think that my suggestion to add a new "is_internal" (or whatever) new DB field could be better? 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 :) 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. Can we revive this? (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. I had to get help deleting a couple of accidental categories. Anyone still interested in finishing this up? *** Bug 25275 has been marked as a duplicate of this bug. *** 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) Created attachment 103834 [details] [review] Bug 17355: DBIC changes Created attachment 103835 [details] [review] Bug 17355: DBIC changes - boolean flag Created attachment 103836 [details] [review] Bug 17355: Override delete methods Created attachment 103837 [details] [review] Bug 17355: DB changes - set is_system for categories 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. 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.
> 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
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> Created attachment 104100 [details] [review] Bug 17355: DBIC changes Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 104101 [details] [review] Bug 17355: DBIC changes - boolean flag Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 104102 [details] [review] Bug 17355: Override delete methods Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 104103 [details] [review] Bug 17355: DB changes - set is_system for categories Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> 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> 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. 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> 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> 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> 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> 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> 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> 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> 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' Nice work everyone! Pushed to master for 20.05 Created attachment 104352 [details] [review] Bug 17355: Fix failing tests This patch makes the tests pass without deleting mandatory AV cats Created attachment 104539 [details] [review] Bug 17355: (QA follow-up) Fix used package name Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> not backported to 19.11 *** Bug 11267 has been marked as a duplicate of this bug. *** |