|
Description
Jonathan Druart
2020-06-12 14:59:08 UTC
How it looks like: https://snipboard.io/FYuAay.jpg https://snipboard.io/CL7vTo.jpg https://snipboard.io/B5aJ7m.jpg https://snipboard.io/HL9lfO.jpg Created attachment 105837 [details] [review] Bug 25728: Create AV when cataloguing a bibliographic record This whole patch set adds the ability to create a new authorised value directly from the cataloguing module. It will prevent the cataloger to leave and reload the page if they need a new one. Test plan: If you are logged in with a patron with the manage_authorised_values subpermission you will be able to: * Edit a bibliographic record * Search for a subfield linked to an authorised values (not the "fake" AV like itemtypes, branches, cn) * Click the dropdown list and start searching for a value that does not exist * Click enter * Fill the fields in the popup * Save => Your dropdown list has the new AV :) Note: This does not work for the advanced editor. It does not have select2 applied to the selects. I have hardly tried but failed. Sponsored-by: Orex Digital Created attachment 105838 [details] [review] Bug 25728: Create AV when adding a new item We do a bit of refactoring to make the code reusable. Test plan: Same as the first patch but when adding/editing an item QA note: There is a warning from the QA tools FAIL koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt FAIL js_in_body A <script> tag found inside head, must be moved to the body (see bug 17858) I don't think how we could avoid it. Sponsored-by: Orex Digital Created attachment 105839 [details] [review] Bug 25728: Create AV when cataloguing an authority record Test plan: Same as the first patch but when cataloguing an authority record Sponsored-by: Orex Digital Created attachment 105840 [details] [review] Bug 25728: Don't explode in the advanced editor Sponsored-by: Orex Digital Created attachment 105841 [details] [review] Bug 25728: Fix the clone field feature Sponsored-by: Orex Digital Created attachment 105842 [details] [review] Bug 25728: Don't explode if no CAN_* var defined Sponsored-by: Orex Digital Created attachment 105852 [details] [review] Bug 25728: Use a svc script instead of the REST API endpoint The try to implement correctly the REST API endpoint for authorised values failed. This patch uses an easy to implement svc script for the POST route. (In reply to Jonathan Druart from comment #1) > How it looks like: > https://snipboard.io/FYuAay.jpg > https://snipboard.io/CL7vTo.jpg > https://snipboard.io/B5aJ7m.jpg > https://snipboard.io/HL9lfO.jpg Pretty! Created attachment 106696 [details] [review] Bug 25728: Create AV when cataloguing a bibliographic record This whole patch set adds the ability to create a new authorised value directly from the cataloguing module. It will prevent the cataloger to leave and reload the page if they need a new one. Test plan: If you are logged in with a patron with the manage_authorised_values subpermission you will be able to: * Edit a bibliographic record * Search for a subfield linked to an authorised values (not the "fake" AV like itemtypes, branches, cn) * Click the dropdown list and start searching for a value that does not exist * Click enter * Fill the fields in the popup * Save => Your dropdown list has the new AV :) Note: This does not work for the advanced editor. It does not have select2 applied to the selects. I have hardly tried but failed. Sponsored-by: Orex Digital Signed-off-by:Hugo Agud <hagud@orex.es> Created attachment 106697 [details] [review] Bug 25728: Create AV when adding a new item We do a bit of refactoring to make the code reusable. Test plan: Same as the first patch but when adding/editing an item QA note: There is a warning from the QA tools FAIL koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt FAIL js_in_body A <script> tag found inside head, must be moved to the body (see bug 17858) I don't think how we could avoid it. Sponsored-by: Orex Digital Signed-off-by: Your Full Name <your_email> Created attachment 106698 [details] [review] Bug 25728: Create AV when cataloguing an authority record Test plan: Same as the first patch but when cataloguing an authority record Sponsored-by: Orex Digital Signed-off-by: Your Full Name <your_email> Created attachment 106699 [details] [review] Bug 25728: Don't explode in the advanced editor Sponsored-by: Orex Digital Signed-off-by: Your Full Name <your_email> Created attachment 106700 [details] [review] Bug 25728: Fix the clone field feature Sponsored-by: Orex Digital Signed-off-by: Your Full Name <your_email> Created attachment 106701 [details] [review] Bug 25728: Don't explode if no CAN_* var defined Sponsored-by: Orex Digital Signed-off-by: Your Full Name <your_email> Created attachment 106702 [details] [review] Bug 25728: Use a svc script instead of the REST API endpoint The try to implement correctly the REST API endpoint for authorised values failed. This patch uses an easy to implement svc script for the POST route. Signed-off-by: Your Full Name <your_email> This depends on bug 25727 that hasn't been signed off yet. There has also been something gone wrong with the sign-off line on these patches here. Created attachment 108356 [details] [review] Bug 25728: Create AV when cataloguing a bibliographic record This whole patch set adds the ability to create a new authorised value directly from the cataloguing module. It will prevent the cataloger to leave and reload the page if they need a new one. Test plan: If you are logged in with a patron with the manage_authorised_values subpermission you will be able to: * Edit a bibliographic record * Search for a subfield linked to an authorised values (not the "fake" AV like itemtypes, branches, cn) * Click the dropdown list and start searching for a value that does not exist * Click enter * Fill the fields in the popup * Save => Your dropdown list has the new AV :) Note: This does not work for the advanced editor. It does not have select2 applied to the selects. I have hardly tried but failed. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108357 [details] [review] Bug 25728: Create AV when adding a new item We do a bit of refactoring to make the code reusable. Test plan: Same as the first patch but when adding/editing an item QA note: There is a warning from the QA tools FAIL koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt FAIL js_in_body A <script> tag found inside head, must be moved to the body (see bug 17858) I don't think how we could avoid it. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108358 [details] [review] Bug 25728: Create AV when cataloguing an authority record Test plan: Same as the first patch but when cataloguing an authority record Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108359 [details] [review] Bug 25728: Don't explode in the advanced editor Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108360 [details] [review] Bug 25728: Fix the clone field feature Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108361 [details] [review] Bug 25728: Don't explode if no CAN_* var defined Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108362 [details] [review] Bug 25728: Use a svc script instead of the REST API endpoint The try to implement correctly the REST API endpoint for authorised values failed. This patch uses an easy to implement svc script for the POST route. Signed-off-by: Hugo Agud <hagud@orex.es> Can you please rebase? Created attachment 108883 [details] [review] Bug 25728: Create AV when cataloguing a bibliographic record This whole patch set adds the ability to create a new authorised value directly from the cataloguing module. It will prevent the cataloger to leave and reload the page if they need a new one. Test plan: If you are logged in with a patron with the manage_authorised_values subpermission you will be able to: * Edit a bibliographic record * Search for a subfield linked to an authorised values (not the "fake" AV like itemtypes, branches, cn) * Click the dropdown list and start searching for a value that does not exist * Click enter * Fill the fields in the popup * Save => Your dropdown list has the new AV :) Note: This does not work for the advanced editor. It does not have select2 applied to the selects. I have hardly tried but failed. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108884 [details] [review] Bug 25728: Create AV when adding a new item We do a bit of refactoring to make the code reusable. Test plan: Same as the first patch but when adding/editing an item QA note: There is a warning from the QA tools FAIL koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt FAIL js_in_body A <script> tag found inside head, must be moved to the body (see bug 17858) I don't think how we could avoid it. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108885 [details] [review] Bug 25728: Create AV when cataloguing an authority record Test plan: Same as the first patch but when cataloguing an authority record Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108886 [details] [review] Bug 25728: Don't explode in the advanced editor Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108887 [details] [review] Bug 25728: Fix the clone field feature Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108888 [details] [review] Bug 25728: Don't explode if no CAN_* var defined Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108889 [details] [review] Bug 25728: Use a svc script instead of the REST API endpoint The try to implement correctly the REST API endpoint for authorised values failed. This patch uses an easy to implement svc script for the POST route. Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 108890 [details] [review] Bug 25728: fix rebase issues (In reply to Katrin Fischer from comment #25) > Can you please rebase? Rebased, that was painful. The QA failure is ok "A <script> tag found inside head, must be moved to the body" I had to put it in head. Hi Joubu, finally get to test here :) 1) QA test tools: FAIL koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt OK filters OK forbidden patterns OK git manipulation FAIL js_in_body A <script> tag found inside head, must be moved to the body (see bug 17858) 2) GUI I would suggest a small change. Most of the codes should best be added capitalized and without special characters. I'd suggest either pre-filling the code or the description with the entered text, leaning towards the description. One argument for description would be that most of the item status fields only work with numerical codes. Also the code field and description should be required fields in the modal. What do you think? Talked to Joubu: - We indeed to not have these fields are required on the admin screen, so we just match the behaviour in the modal. - We also do not keep people from adding values that won't work... so validation should be handled separately as well. - The JS error from the QA script was checked and is ok (see comment#34) I've now tested in authorities and cataloguing too. I still feel like we should not pre-fill the authorised value with the text entered, but only the description. Otherwise this works really nicely. Codes should be picked wisely, I think that would better support this. Created attachment 108922 [details] [review] Bug 25728: Don't prefill av's code Created attachment 108923 [details] [review] Bug 25728: Create AV when cataloguing a bibliographic record This whole patch set adds the ability to create a new authorised value directly from the cataloguing module. It will prevent the cataloger to leave and reload the page if they need a new one. Test plan: If you are logged in with a patron with the manage_authorised_values subpermission you will be able to: * Edit a bibliographic record * Search for a subfield linked to an authorised values (not the "fake" AV like itemtypes, branches, cn) * Click the dropdown list and start searching for a value that does not exist * Click enter * Fill the fields in the popup * Save => Your dropdown list has the new AV :) Note: This does not work for the advanced editor. It does not have select2 applied to the selects. I have hardly tried but failed. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108924 [details] [review] Bug 25728: Create AV when adding a new item We do a bit of refactoring to make the code reusable. Test plan: Same as the first patch but when adding/editing an item QA note: There is a warning from the QA tools FAIL koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt FAIL js_in_body A <script> tag found inside head, must be moved to the body (see bug 17858) I don't think how we could avoid it. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108925 [details] [review] Bug 25728: Create AV when cataloguing an authority record Test plan: Same as the first patch but when cataloguing an authority record Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108926 [details] [review] Bug 25728: Don't explode in the advanced editor Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108927 [details] [review] Bug 25728: Fix the clone field feature Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108928 [details] [review] Bug 25728: Don't explode if no CAN_* var defined Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108929 [details] [review] Bug 25728: Use a svc script instead of the REST API endpoint The try to implement correctly the REST API endpoint for authorised values failed. This patch uses an easy to implement svc script for the POST route. Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108930 [details] [review] Bug 25728: fix rebase issues Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108931 [details] [review] Bug 25728: Don't prefill av's code Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 20.11, thanks to everybody involved! missing dependencies for 20.05.x, no backport I don't like this patch at all! After just a few weeks we have dozens of new locations created by mistake, like HORS, FICT, NON-FI &c, &c Staff type just the first letters of a location and then hit enter, and voila, there is a new location! Please make a syspref for this, and make it disabled by default - or set it as a permission that could be turned off, even for superlibrarians (In reply to Klas Blomberg from comment #50) > I don't like this patch at all! > After just a few weeks we have dozens of new locations created by mistake, > like HORS, FICT, NON-FI &c, &c > > Staff type just the first letters of a location and then hit enter, and > voila, there is a new location! > > Please make a syspref for this, and make it disabled by default - or set it > as a permission that could be turned off, even for superlibrarians Hi Klas, this is not how it should work. The intended behaviour is that it opens up a modal where you have to enter further information - code and descripton for the new authorised value. If it works like you describe, this would be a bug. It is already tied to a permission - you can't add them if you don't have managing authorised values permission. Does that help? Also: Filing a new bug and linking to the existing bug once released is better, we will have to do this for this to progress and with the link everyone added here will also get emailed. |
