Description
Jesse Weaver
2016-02-09 19:46:47 UTC
Jesse, since we will have 3 different places to configure the additional fields, it would be great to have a configuration page in the admin area to configure all of them on the same page. That's exactly the plan, Jonathan. Created attachment 48224 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. The tablename is not saved when I create a new field for aqbasket. Note that I would prefer to this patch based on top of bug 11844. I'll fix these issues, but could I ask you to prepare a patch to rebase bug 11844? There's a bit of mucking around that has to be done that I'd prefer was done by somebody familiar with the code. I'll happily QA it. I have changed the status of bug 11844 to "does not apply". Hopefully Julian will rebase it quickly. Created attachment 53608 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. Created attachment 53609 [details] [review] Bug 15774: (QA followup) Make sure that tablename is correctly saved This should fix QA issues, rebasing 11844 shortly. QA comments: 1/ Regression: On serials/serials-search.pl, the filters are not filled with what the users searched for and the additional fields linked to an AV are not displayed as a dropdown list. 2/ What does mean an additional field linked to a MARC field for aqbasket? I think that this option should be hidden for this table (additional fields admin page, /acqui/histsearch.pl, add basket page, etc.) 3/ No test coverage for update_fields_from_query, get_filters_from_query and get_filters_as_values. 4/ When searching an order, there is no way not to set a "all" value for additional fields linked to AV. 5/ When searching an order and the search does not return any results, the field should contain what the user filled in. 6/ update_fields_from_query and get_filters_from_query should not deal with the CGI object. It would be better to pass the relevant variables instead. Marked as Failed QA. Can this be rescued? Created attachment 72078 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. Created attachment 72079 [details] [review] Bug 15774: (QA followup) Make sure that tablename is correctly saved Patches rebased on master QA comments in comment 10 still need to be addressed. Created attachment 72085 [details] [review] Bug 15774: Fix AV categories dropdown list in admin Created attachment 72086 [details] [review] Bug 15774: Fix additional fields filters Addresses the following QA comments: 1/ Regression: On serials/serials-search.pl, the filters are not filled with what the users searched for and the additional fields linked to an AV are not displayed as a dropdown list. 4/ When searching an order, there is no way not to set a "all" value for additional fields linked to AV. 5/ When searching an order and the search does not return any results, the field should contain what the user filled in. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15774#c10 (In reply to Jonathan Druart from comment #10) > 2/ What does mean an additional field linked to a MARC field for aqbasket? > I think that this option should be hidden for this table (additional fields > admin page, /acqui/histsearch.pl, add basket page, etc.) I think we should answer to that before fixing the other issues. Changing status to In Discussion I agree with Jonathan, I don't think that option makes sense and should be hidden/disabled for basket fields. Great - follow up patch? :) (In reply to Katrin Fischer from comment #19) > Great - follow up patch? :) Will attempt, having some issues in the code right now, will dig later Created attachment 75083 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. Created attachment 75084 [details] [review] Bug 15774: (QA followup) Make sure that tablename is correctly saved Created attachment 75085 [details] [review] Bug 15774: Fix additional fields filters Addresses the following QA comments: 1/ Regression: On serials/serials-search.pl, the filters are not filled with what the users searched for and the additional fields linked to an AV are not displayed as a dropdown list. 4/ When searching an order, there is no way not to set a "all" value for additional fields linked to AV. 5/ When searching an order and the search does not return any results, the field should contain what the user filled in. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15774#c10 Created attachment 75086 [details] [review] Bug 15774: Use Koha::Object(s) for additional fields A lot of code can be removed just by using Koha::Object It also makes fetching and updating additional field values easier. Patches rebased on master + new patch (attachment 75086 [details] [review]) Reset bug status to Needs signoff since the last patch brings a lot of changes. To test, use test plan from comment 21 Created attachment 75336 [details] [review] Bug 15774: Add POD and license notices Created attachment 75354 [details] [review] Bug 15774: Show additional field values in edit form Created attachment 75361 [details] [review] Bug 15774: Show additional field values in edit form i tried to rebase this... but failed :( The method Koha::Subscription->additional_field_values is not covered by tests! Nick, could you give this a go? Created attachment 79976 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. Created attachment 79977 [details] [review] Bug 15774: (QA follow-up) Make sure that tablename is correctly saved Created attachment 79978 [details] [review] Bug 15774: Fix additional fields filters Addresses the following QA comments: 1/ Regression: On serials/serials-search.pl, the filters are not filled with what the users searched for and the additional fields linked to an AV are not displayed as a dropdown list. 4/ When searching an order, there is no way not to set a "all" value for additional fields linked to AV. 5/ When searching an order and the search does not return any results, the field should contain what the user filled in. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15774#c10 Created attachment 79979 [details] [review] Bug 15774: Use Koha::Object(s) for additional fields A lot of code can be removed just by using Koha::Object It also makes fetching and updating additional field values easier. Created attachment 79980 [details] [review] Bug 15774: Add POD and license notices Created attachment 79981 [details] [review] Bug 15774: Show additional field values in edit form Created attachment 79982 [details] [review] Bug 15774: (follow-up) Add html filters Created attachment 80060 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 80061 [details] [review] Bug 15774: (QA follow-up) Make sure that tablename is correctly saved Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 80062 [details] [review] Bug 15774: Fix additional fields filters Addresses the following QA comments: 1/ Regression: On serials/serials-search.pl, the filters are not filled with what the users searched for and the additional fields linked to an AV are not displayed as a dropdown list. 4/ When searching an order, there is no way not to set a "all" value for additional fields linked to AV. 5/ When searching an order and the search does not return any results, the field should contain what the user filled in. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15774#c10 Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 80063 [details] [review] Bug 15774: Use Koha::Object(s) for additional fields A lot of code can be removed just by using Koha::Object It also makes fetching and updating additional field values easier. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 80064 [details] [review] Bug 15774: Add POD and license notices Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 80065 [details] [review] Bug 15774: Show additional field values in edit form Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 80066 [details] [review] Bug 15774: (follow-up) Add html filters Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 80680 [details] [review] Bug 15774: Fix JS check when AV and MARC field are both selected * Tests missing for GetHistory * + my $rs = Koha::Database->new->schema->resultset('AdditionalFieldValue'); must be Koha::AdditionalFieldValue instead. I would suggest to have a look at Koha::Patron::Category and how the 'branch_limitation' add/update/remove is done, in order to follow an existing pattern * additional_field_values must return a Koha::Objects * Should not search_additional_fields be more search_*by*_additional_fields? I think we agreed on "filter_by_*" somewhere else (Martin?) * + # TODO Add column additional_field_values.tablename to avoid subquery ? I do not think it is relevant, we want the tablename at additional_fields level * + for my $field (Koha::AdditionalFields->search({ tablename => 'aqbasket' })) { Better to use while ->next instead These patches are great, please provide a quick follow-up and we will make it part of 18.11! I think search_by doesn't sound quite right... search_on? search_in? Native speaker? :) How's about 'filter_by' as per the suggestions/discussion over on bug 11983 ? (In reply to Katrin Fischer from comment #46) > I think search_by doesn't sound quite right... search_on? search_in? Native > speaker? :) search_for? I don't like filter_by, because filter_by suggests we are taking a given set and shrinking it. No such thing is passed. We are searching, but it is limited for a particular set of conditions. The rest would describe the condition. And why not just get_by_marc_field (would do tagfield, but tagsubfield is included, I believe), get_by_kohafield? Created attachment 81490 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 81491 [details] [review] Bug 15774: (QA follow-up) Make sure that tablename is correctly saved Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 81492 [details] [review] Bug 15774: Fix additional fields filters Addresses the following QA comments: 1/ Regression: On serials/serials-search.pl, the filters are not filled with what the users searched for and the additional fields linked to an AV are not displayed as a dropdown list. 4/ When searching an order, there is no way not to set a "all" value for additional fields linked to AV. 5/ When searching an order and the search does not return any results, the field should contain what the user filled in. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15774#c10 Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 81493 [details] [review] Bug 15774: Use Koha::Object(s) for additional fields A lot of code can be removed just by using Koha::Object It also makes fetching and updating additional field values easier. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 81494 [details] [review] Bug 15774: Add POD and license notices Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 81495 [details] [review] Bug 15774: Show additional field values in edit form Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 81496 [details] [review] Bug 15774: (follow-up) Add html filters Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 81497 [details] [review] Bug 15774: Fix JS check when AV and MARC field are both selected (In reply to M. Tompsett from comment #48) > (In reply to Katrin Fischer from comment #46) > > I think search_by doesn't sound quite right... search_on? search_in? Native > > speaker? :) > > search_for? I don't like filter_by, because filter_by suggests we are taking > a given set and shrinking it. No such thing is passed. We are searching, but > it is limited for a particular set of conditions. The rest would describe > the condition. And why not just get_by_marc_field (would do tagfield, but > tagsubfield is included, I believe), get_by_kohafield? But you are filtering... you pass in a resultset (whether that be a new one or an existing already partially filtered one) and then refine it by filtering down... We're trying to bring consistency to the objects, so to me filter_by which is already being discussed elsewhere seems to make sense. Created attachment 81833 [details] [review] Bug 15774: (follow-up) Address QA issues Created attachment 81837 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81838 [details] [review] Bug 15774: (QA follow-up) Make sure that tablename is correctly saved Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81839 [details] [review] Bug 15774: Fix additional fields filters Addresses the following QA comments: 1/ Regression: On serials/serials-search.pl, the filters are not filled with what the users searched for and the additional fields linked to an AV are not displayed as a dropdown list. 4/ When searching an order, there is no way not to set a "all" value for additional fields linked to AV. 5/ When searching an order and the search does not return any results, the field should contain what the user filled in. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15774#c10 Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81840 [details] [review] Bug 15774: Use Koha::Object(s) for additional fields A lot of code can be removed just by using Koha::Object It also makes fetching and updating additional field values easier. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81841 [details] [review] Bug 15774: Add POD and license notices Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81842 [details] [review] Bug 15774: Show additional field values in edit form Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81843 [details] [review] Bug 15774: (follow-up) Add html filters Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81844 [details] [review] Bug 15774: Fix JS check when AV and MARC field are both selected Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81845 [details] [review] Bug 15774: (follow-up) Address QA issues Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81846 [details] [review] Bug 15774: Use the include file to display the subscription's additional fields We have it, why not to use it? Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81847 [details] [review] Bug 15774: Fix insert and update for subscription Call it a typo Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81848 [details] [review] Bug 15774: Add missing tests for GetSubscription Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81849 [details] [review] Bug 15774: Add missing tests for Koha::Object::Mixin::AdditionalFields Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81850 [details] [review] Bug 15774: Fix and unify additional_fields in C4::Serials Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81851 [details] [review] Bug 15774: Do not fetch additional fields from GetSubscription Finally we do not want that, they should be fetch when needed Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81852 [details] [review] Bug 15774: Remove unecessary returned value for ->delete Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Comment on attachment 81848 [details] [review] Bug 15774: Add missing tests for GetSubscription We finally do not need that I'd say. This needs a new permission now to reflect changes made by 14391 (In reply to Nick Clemens from comment #76) > This needs a new permission now to reflect changes made by 14391 Hm, are these under administration now? Some conflicts: CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/histsearch.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/histsearch.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc Auto-merging admin/additional-fields.pl Auto-merging acqui/histsearch.pl CONFLICT (content): Merge conflict in acqui/histsearch.pl Auto-merging acqui/basket.pl Auto-merging C4/Acquisition.pm CONFLICT (content): Merge conflict in C4/Acquisition.pm error: Failed to merge in the changes. For permissions: Until this patch everyone with any serials permission could add/update additional fields: { serials => '*' }, It's a little hard to find a good pref for a feature that spans multiple-modules now. I think a new admin permission might make the most sense. Then update patrons with serials permissions to have that. Patches rebased: https://gitlab.com/joubu/Koha/commits/bug_15774 Created attachment 83349 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83350 [details] [review] Bug 15774: (QA follow-up) Make sure that tablename is correctly saved Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83351 [details] [review] Bug 15774: Fix additional fields filters Addresses the following QA comments: 1/ Regression: On serials/serials-search.pl, the filters are not filled with what the users searched for and the additional fields linked to an AV are not displayed as a dropdown list. 4/ When searching an order, there is no way not to set a "all" value for additional fields linked to AV. 5/ When searching an order and the search does not return any results, the field should contain what the user filled in. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15774#c10 Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83352 [details] [review] Bug 15774: Use Koha::Object(s) for additional fields A lot of code can be removed just by using Koha::Object It also makes fetching and updating additional field values easier. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83353 [details] [review] Bug 15774: Add POD and license notices Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83354 [details] [review] Bug 15774: Show additional field values in edit form Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83355 [details] [review] Bug 15774: (follow-up) Add html filters Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83356 [details] [review] Bug 15774: Fix JS check when AV and MARC field are both selected Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83357 [details] [review] Bug 15774: (follow-up) Address QA issues Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83358 [details] [review] Bug 15774: Use the include file to display the subscription's additional fields We have it, why not to use it? Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83359 [details] [review] Bug 15774: Fix insert and update for subscription Call it a typo Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83360 [details] [review] Bug 15774: Add missing tests for Koha::Object::Mixin::AdditionalFields Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83361 [details] [review] Bug 15774: Fix and unify additional_fields in C4::Serials Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83362 [details] [review] Bug 15774: Do not fetch additional fields from GetSubscription Finally we do not want that, they should be fetch when needed Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83363 [details] [review] Bug 15774: Remove unecessary returned value for ->delete Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83364 [details] [review] Bug 15774: Add permission for managing additional fields To test: 1 - Have three borrowers, one with order_manage permission, one with edit_subscription permisson, andone with both 2 - Apply patch, updatedatabase 3 - Verify all three now have the manage_additional_fields permission 4 - Visit the admin page with these users, they should all see the 'Manage additional fields' link 5 - Click the link 6 - User with order_manage should see 'Order baskest' 7 - User with edit_subscription should see 'Subscriptions' 8 - User with both should see both 9 - Remove the additional permissions from a user - they should see a note about needing additional permissions Comment on attachment 83364 [details] [review] Bug 15774: Add permission for managing additional fields Review of attachment 83364 [details] [review]: ----------------------------------------------------------------- Hi Nick, I like he approach of the new permission depending on order_manage and edit_subscription, but I think it's not quite ready yet. Trying to comment with Splinter. 1) You need to update installer/data/mysql/userpermissions.sql so the permission is installed for new installations. ::: admin/additional-fields.pl @@ +33,5 @@ > +my $op = $input->param('op') // ( $tablename ? 'list' : 'list_tables' ); > + > +if( $op ne 'list_tables' ){ > + $flagsrequired{acquisition} = 'order_manage' if $tablename eq 'aqbasket'; > + $flagsrequired{serials} = 'edit_subscription' if $tablename eq 'subscription'; 2) I think you'd want to change this to use the new permission now. ::: installer/data/mysql/atomicupdate/Bug_15774_add_additional_fields_permissions.perl @@ +6,5 @@ > + (3,'manage_additional_fields','Add, edit, or delete additional custom fields for baskets or subscriptions (also requires order_manage or edit_subscription permissions)') > + }); > + $dbh->do( q{ > + INSERT INTO user_permissions (borrowernumber, module_bit, code) > + SELECT borrowernumber, 3, 'manage_additional_fields' FROM borrowers WHERE borrowernumber IN (SELECT DISTINCT borrowernumber FROM user_permissions WHERE code = 'order_manage' OR code = 'edit_subscription'); 3) This will work for everyone who has not full Serials permission. If the top level "Serials" permission is checked, the granular ones are not in user_permissions. You also need to check for those using the bit operation. Created attachment 83444 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83445 [details] [review] Bug 15774: (QA follow-up) Make sure that tablename is correctly saved Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83446 [details] [review] Bug 15774: Fix additional fields filters Addresses the following QA comments: 1/ Regression: On serials/serials-search.pl, the filters are not filled with what the users searched for and the additional fields linked to an AV are not displayed as a dropdown list. 4/ When searching an order, there is no way not to set a "all" value for additional fields linked to AV. 5/ When searching an order and the search does not return any results, the field should contain what the user filled in. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15774#c10 Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83447 [details] [review] Bug 15774: Use Koha::Object(s) for additional fields A lot of code can be removed just by using Koha::Object It also makes fetching and updating additional field values easier. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83448 [details] [review] Bug 15774: Add POD and license notices Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83449 [details] [review] Bug 15774: Show additional field values in edit form Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83450 [details] [review] Bug 15774: (follow-up) Add html filters Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83451 [details] [review] Bug 15774: Fix JS check when AV and MARC field are both selected Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83452 [details] [review] Bug 15774: (follow-up) Address QA issues Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83453 [details] [review] Bug 15774: Use the include file to display the subscription's additional fields We have it, why not to use it? Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83454 [details] [review] Bug 15774: Fix insert and update for subscription Call it a typo Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83455 [details] [review] Bug 15774: Add missing tests for Koha::Object::Mixin::AdditionalFields Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83456 [details] [review] Bug 15774: Fix and unify additional_fields in C4::Serials Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83457 [details] [review] Bug 15774: Do not fetch additional fields from GetSubscription Finally we do not want that, they should be fetch when needed Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83458 [details] [review] Bug 15774: Remove unecessary returned value for ->delete Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 83459 [details] [review] Bug 15774: Add permission for managing additional fields To test: 1 - Have three borrowers, one with order_manage permission, one with edit_subscription permisson, andone with both 2 - Apply patch, updatedatabase 3 - Verify all three now have the manage_additional_fields permission 4 - Visit the admin page with these users, they should all see the 'Manage additional fields' link 5 - Click the link 6 - User with order_manage should see 'Order baskest' 7 - User with edit_subscription should see 'Subscriptions' 8 - User with both should see both 9 - Remove the additional permissions from a user - they should see a note about needing additional permissions ah, I think I understand 2) now. (In reply to Katrin Fischer from comment #96) > I like the approach of the new permission depending on order_manage and > edit_subscription Excellent! > 1) You need to update installer/data/mysql/userpermissions.sql so the > permission is installed for new installations. Done! > ::: admin/additional-fields.pl > @@ +33,5 @@ > > +my $op = $input->param('op') // ( $tablename ? 'list' : 'list_tables' ); > > + > > +if( $op ne 'list_tables' ){ > > + $flagsrequired{acquisition} = 'order_manage' if $tablename eq 'aqbasket'; > > + $flagsrequired{serials} = 'edit_subscription' if $tablename eq 'subscription'; > > 2) I think you'd want to change this to use the new permission now. I do set it in the lines above - the main permission gets you to the page, you need the others to access the individual modules > > ::: > installer/data/mysql/atomicupdate/ > Bug_15774_add_additional_fields_permissions.perl > @@ +6,5 @@ > > + (3,'manage_additional_fields','Add, edit, or delete additional custom fields for baskets or subscriptions (also requires order_manage or edit_subscription permissions)') > > + }); > > + $dbh->do( q{ > > + INSERT INTO user_permissions (borrowernumber, module_bit, code) > > + SELECT borrowernumber, 3, 'manage_additional_fields' FROM borrowers WHERE borrowernumber IN (SELECT DISTINCT borrowernumber FROM user_permissions WHERE code = 'order_manage' OR code = 'edit_subscription'); > > 3) This will work for everyone who has not full Serials permission. If the > top level "Serials" permission is checked, the granular ones are not in > user_permissions. You also need to check for those using the bit operation. Updated I am sorry, but there are some conflicts. Please rebase, I'll try to come back to this soon: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 15774: Add additional fields to order baskets Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/includes/serials-menu.inc M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt M koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt M koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt M koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt M serials/serials-search.pl M serials/subscription-add.pl Falling back to patching base and 3-way merge... Auto-merging serials/subscription-add.pl CONFLICT (content): Merge conflict in serials/subscription-add.pl Auto-merging serials/serials-search.pl CONFLICT (content): Merge conflict in serials/serials-search.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt Removing koha-tmpl/intranet-tmpl/prog/en/modules/serials/add_fields.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/serials-menu.inc CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/serials-menu.inc Auto-merging admin/additional-fields.pl error: Failed to merge in the changes. Patch failed at 0001 Bug 15774: Add additional fields to order baskets The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-15774-Add-additional-fields-to-order-baskets-LGaSot.patch Created attachment 85333 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85334 [details] [review] Bug 15774: (QA follow-up) Make sure that tablename is correctly saved Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85335 [details] [review] Bug 15774: Fix additional fields filters Addresses the following QA comments: 1/ Regression: On serials/serials-search.pl, the filters are not filled with what the users searched for and the additional fields linked to an AV are not displayed as a dropdown list. 4/ When searching an order, there is no way not to set a "all" value for additional fields linked to AV. 5/ When searching an order and the search does not return any results, the field should contain what the user filled in. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15774#c10 Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85336 [details] [review] Bug 15774: Use Koha::Object(s) for additional fields A lot of code can be removed just by using Koha::Object It also makes fetching and updating additional field values easier. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85337 [details] [review] Bug 15774: Add POD and license notices Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85338 [details] [review] Bug 15774: Show additional field values in edit form Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85339 [details] [review] Bug 15774: (follow-up) Add html filters Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85340 [details] [review] Bug 15774: Fix JS check when AV and MARC field are both selected Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85341 [details] [review] Bug 15774: (follow-up) Address QA issues Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85342 [details] [review] Bug 15774: Use the include file to display the subscription's additional fields We have it, why not to use it? Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85343 [details] [review] Bug 15774: Fix insert and update for subscription Call it a typo Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85344 [details] [review] Bug 15774: Add missing tests for Koha::Object::Mixin::AdditionalFields Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85345 [details] [review] Bug 15774: Fix and unify additional_fields in C4::Serials Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85346 [details] [review] Bug 15774: Do not fetch additional fields from GetSubscription Finally we do not want that, they should be fetch when needed Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85347 [details] [review] Bug 15774: Remove unecessary returned value for ->delete Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 85348 [details] [review] Bug 15774: Add permission for managing additional fields To test: 1 - Have three borrowers, one with order_manage permission, one with edit_subscription permisson, andone with both 2 - Apply patch, updatedatabase 3 - Verify all three now have the manage_additional_fields permission 4 - Visit the admin page with these users, they should all see the 'Manage additional fields' link 5 - Click the link 6 - User with order_manage should see 'Order baskest' 7 - User with edit_subscription should see 'Subscriptions' 8 - User with both should see both 9 - Remove the additional permissions from a user - they should see a note about needing additional permissions Sorry, but QA tools are not happy: FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/additional-fields.tt FAIL forbidden patterns forbidden pattern: Attribute type should not be used for script tags (bug 20053) (line 180) FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt FAIL forbidden patterns forbidden pattern: merge marker (=======) (line 212) forbidden pattern: merge marker (>>>>>>>) (line 214) forbidden pattern: merge marker (<<<<<<<) (line 206) FAIL valid_template parse error - /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt line 225: unexpected end of input FAIL serials/serials-search.pl FAIL valid serials/serials-search.pl had compilation errors. Global symbol "$additional_fields" requires explicit package name (did you forget to declare "my $additional_fields"?) Global symbol "$additional_field_filters" requires explicit package name (did you forget to declare "my $additional_field_filters"?) Also, I have one concern: this patchset does much more than this report describes. I would like to see, at least the refactoring part of this code (change to Koha::Object and adapt current codebase on it) as own report. (In reply to Josef Moravec from comment #132) > Also, I have one concern: this patchset does much more than this report > describes. > I would like to see, at least the refactoring part of this code (change to > Koha::Object and adapt current codebase on it) as own report. The patch set is huge, and the dependency with bug 11844 tricky. I would not suggest such changes as we may broke everything (again). What advantage(s) could it bring here? /broke/break/ Created attachment 85792 [details] [review] Bug 15774: (follow-up) fix rebase issues (In reply to Josef Moravec from comment #132) > Sorry, but QA tools are not happy: Fixed QA tool errors (In reply to Jonathan Druart from comment #133) > (In reply to Josef Moravec from comment #132) > > Also, I have one concern: this patchset does much more than this report > > describes. > > I would like to see, at least the refactoring part of this code (change to > > Koha::Object and adapt current codebase on it) as own report. > > The patch set is huge, and the dependency with bug 11844 tricky. I would not > suggest such changes as we may broke everything (again). > > What advantage(s) could it bring here? Thanks for discussion, I will not block it then. Created attachment 85806 [details] [review] Bug 15774: Add additional fields to order baskets This also moves the admin page for additional fields for all tables to a single common screen, and factors out display/input parsing logic. Test plan: 1. Create an additional field for a subscription (under Serials -> Add subscription fields). 2. Apply patch. 3. Visit Additional fields under administration, and verify that the field created above still shows under the list for the subscription table. 4. Create at least four fields for aqbasket for each combination of searchable/not-searchable and with/without an authorized value. 5. Create an order basket, and verify that all fields are visible and correctly save. 6. Edit the basket, verifying that changes to these additional fields are saved. 7. Add an order to the basket (contents are irrelevant). 8. Go to advanced search within acquisitions. 9. Verify that only the searchable fields show in the form, and that their contents may be searched. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85807 [details] [review] Bug 15774: (QA follow-up) Make sure that tablename is correctly saved Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85808 [details] [review] Bug 15774: Fix additional fields filters Addresses the following QA comments: 1/ Regression: On serials/serials-search.pl, the filters are not filled with what the users searched for and the additional fields linked to an AV are not displayed as a dropdown list. 4/ When searching an order, there is no way not to set a "all" value for additional fields linked to AV. 5/ When searching an order and the search does not return any results, the field should contain what the user filled in. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15774#c10 Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85809 [details] [review] Bug 15774: Use Koha::Object(s) for additional fields A lot of code can be removed just by using Koha::Object It also makes fetching and updating additional field values easier. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85810 [details] [review] Bug 15774: Add POD and license notices Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85811 [details] [review] Bug 15774: Show additional field values in edit form Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85812 [details] [review] Bug 15774: (follow-up) Add html filters Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85813 [details] [review] Bug 15774: Fix JS check when AV and MARC field are both selected Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85814 [details] [review] Bug 15774: (follow-up) Address QA issues Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85815 [details] [review] Bug 15774: Use the include file to display the subscription's additional fields We have it, why not to use it? Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85816 [details] [review] Bug 15774: Fix insert and update for subscription Call it a typo Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85817 [details] [review] Bug 15774: Add missing tests for Koha::Object::Mixin::AdditionalFields Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85818 [details] [review] Bug 15774: Fix and unify additional_fields in C4::Serials Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85819 [details] [review] Bug 15774: Do not fetch additional fields from GetSubscription Finally we do not want that, they should be fetch when needed Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85820 [details] [review] Bug 15774: Remove unecessary returned value for ->delete Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85821 [details] [review] Bug 15774: Add permission for managing additional fields To test: 1 - Have three borrowers, one with order_manage permission, one with edit_subscription permisson, andone with both 2 - Apply patch, updatedatabase 3 - Verify all three now have the manage_additional_fields permission 4 - Visit the admin page with these users, they should all see the 'Manage additional fields' link 5 - Click the link 6 - User with order_manage should see 'Order baskest' 7 - User with edit_subscription should see 'Subscriptions' 8 - User with both should see both 9 - Remove the additional permissions from a user - they should see a note about needing additional permissions Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85822 [details] [review] Bug 15774: (follow-up) fix rebase issues Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85823 [details] [review] Bug 15774: (QA follow-up) Fix use statemenets Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85824 [details] [review] Bug 15774: (QA follow-up) Remove asset of datables.css, and corresponding plugins It is now part of doc-head-close.inc Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Awesome work all! Pushed to master for 19.05 Created attachment 86411 [details] [review] Bug 15774: (follow-up) Fix TestBuilder.t so it doesn't catch wrong classes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> New feature will not be backported to 18.11.x series |