|
Description
Joy Nelson
2019-05-30 20:33:56 UTC
Created attachment 104972 [details] [review] Bug 23019: Update database structure and add atomic update Created attachment 104973 [details] [review] Bug 23019: Add koha_object_class and koha_objects_class methods to ImportBatch.pm schema Created attachment 104974 [details] [review] Bug 23019: Add ImportBatchesProfile schema and set is_boolean to parse_items column Created attachment 104975 [details] [review] Bug 23019: Add tests Created attachment 104976 [details] [review] Bug 23019: Add profiles to stage-import-batch and magnage-import-batch pages This patch adds the logic and the needed UI elements to be able to pre-load an import profile. It also displays which profile was used to stage an import in staged import manager. To test: 1. Apply all patches 2. Updatedatabase 3. Go to Stage MARC records for Import tool in admin, and upload a file with MARC records. CHECK => after uploading, there is a fieldset with the legend “Profile settings” => inside the fieldset there is a select labeled “Pre fill values with profile”. The only value it has is “Do not use profile”. 4. Change some settings, and set “profile 1” as profile name and click on “Add profile” SUCCESS => The select now has the new profile selected 5. Change profile select to “Do not use profile” SUCCESS => Default values are now displayed in the form 6. Reload the page and upload the file again SUCCESS => the select still has the profile recently added 7. Select the profile, change some parameter in the form and set the profile name to “profile 2”, and click add profile SUCCESS => there are two profiles now, and if you toggle between them, the parameter changes 8. Select profile 1, change one parameter and click on update profile SUCCESS => if you toggle that profile with the other, the new parameter of the value is shown when you select profile 1 9. Select profile 2, change some parameter and click Add profile (leaving the name as profile 2) SUCCESS => the page complains there is another profile with the same name, and asks if you want to replace it. 10. Click on accept SUCCESS => profile 2 now has the new value in the parameter 11. Select profile 2 and change the name to profile 1 SUCCESS => the page complains there is another profile with that name, and asks if you want to replace it 12. Click on accept SUCCESS => in profile select there is only one profile called profile 1 that has the values of profile 2 13. Select profile 1 and click remove profile SUCCESS => there is no profile in profile select. 14. prove t/db_dependent/ImportBatch.t t/db_dependent/api/v1/import_batch_profiles.t 15. Sign off Yes! I've been meaning to do something like this for years. I've even been thinking about implementing it for https://github.com/minusdavid/koha-plugin-oaipmh-import, so it would be great to have this in the core codebase! I'll make a note to come back to this, but very very interested. Comment on attachment 104976 [details] [review] Bug 23019: Add profiles to stage-import-batch and magnage-import-batch pages Review of attachment 104976 [details] [review]: ----------------------------------------------------------------- ::: api/v1/swagger/definitions/import-batch-profile.json @@ +9,5 @@ > + "description": "name of this profile", > + "type": "string" > + }, > + "matcher_id": { > + "description": "the id of the match rule used (matchpoints.matcher_id)", Wouldn't marc_matchers.matcher_id be a more accurate table description here? (In reply to David Cook from comment #6) > Yes! I've been meaning to do something like this for years. > > I've even been thinking about implementing it for > https://github.com/minusdavid/koha-plugin-oaipmh-import, so it would be > great to have this in the core codebase! > Looks like this won't be a good fit for the plugin but interesting nonetheless! Change back to Assigned because just discovered issues to fix This sounds really slick. I'm looking forward to testing it! I tried creating a sandbox with this bug number to see what the development looks like in action, but the staff client won't load for me right now. Created attachment 105536 [details] [review] Bug 23019: Update database structure and add atomic update Created attachment 105537 [details] [review] Bug 23019: Add koha_object_class and koha_objects_class methods to ImportBatch.pm schema Created attachment 105538 [details] [review] Bug 23019: Add ImportBatchesProfile schema and set is_boolean to parse_items column Created attachment 105539 [details] [review] Bug 23019: Add tests Created attachment 105540 [details] [review] Bug 23019: Add profiles to stage-import-batch and magnage-import-batch pages This patch adds the logic and the needed UI elements to be able to pre-load an import profile. It also displays which profile was used to stage an import in staged import manager. To test: 1. Apply all patches 2. Updatedatabase 3. Go to Stage MARC records for Import tool in admin, and upload a file with MARC records. CHECK => after uploading, there is a fieldset with the legend “Profile settings” => inside the fieldset there is a select labeled “Pre fill values with profile”. The only value it has is “Do not use profile”. 4. Change some settings, and set “profile 1” as profile name and click on “Add profile” SUCCESS => The select now has the new profile selected 5. Change profile select to “Do not use profile” SUCCESS => Default values are now displayed in the form 6. Reload the page and upload the file again SUCCESS => the select still has the profile recently added 7. Select the profile, change some parameter in the form and set the profile name to “profile 2”, and click add profile SUCCESS => there are two profiles now, and if you toggle between them, the parameter changes 8. Select profile 1, change one parameter and click on update profile SUCCESS => if you toggle that profile with the other, the new parameter of the value is shown when you select profile 1 9. Select profile 2, change some parameter and click Add profile (leaving the name as profile 2) SUCCESS => the page complains there is another profile with the same name, and asks if you want to replace it. 10. Click on accept SUCCESS => profile 2 now has the new value in the parameter 11. Select profile 2 and change the name to profile 1 SUCCESS => the page complains there is another profile with that name, and asks if you want to replace it 12. Click on accept SUCCESS => in profile select there is only one profile called profile 1 that has the values of profile 2 13. Select profile 1 and click remove profile SUCCESS => there is no profile in profile select. 14. Create a profile and click on “Stage for import” 15. Go to Staged MARC management page SUCCESS => Improt should have the name of the profile in profile column, and when you click on the file name, there should be the name of the profile in the details. 16. prove t/db_dependent/ImportBatch.t t/db_dependent/api/v1/import_batch_profiles.t 17. Sign off This is great! There is one area where my matching preferences would not stick when I created a profile. If I select "No" under "Check for embedded item record data" and click "Add profile", No switches back to Yes. My selection for "how to process items" is remembered. It is just the yes-no button that isn't being saved. Created attachment 105541 [details] [review] Bug 23019: Add profiles to stage-import-batch and magnage-import-batch pages This patch adds the logic and the needed UI elements to be able to pre-load an import profile. It also displays which profile was used to stage an import in staged import manager. To test: 1. Apply all patches 2. Updatedatabase 3. Go to Stage MARC records for Import tool in admin, and upload a file with MARC records. CHECK => after uploading, there is a fieldset with the legend “Profile settings” => inside the fieldset there is a select labeled “Pre fill values with profile”. The only value it has is “Do not use profile”. 4. Change some settings, and set “profile 1” as profile name and click on “Add profile” SUCCESS => The select now has the new profile selected 5. Change profile select to “Do not use profile” SUCCESS => Default values are now displayed in the form 6. Reload the page and upload the file again SUCCESS => the select still has the profile recently added 7. Select the profile, change some parameter in the form and set the profile name to “profile 2”, and click add profile SUCCESS => there are two profiles now, and if you toggle between them, the parameter changes 8. Select profile 1, change one parameter and click on update profile SUCCESS => if you toggle that profile with the other, the new parameter of the value is shown when you select profile 1 9. Select profile 2, change some parameter and click Add profile (leaving the name as profile 2) SUCCESS => the page complains there is another profile with the same name, and asks if you want to replace it. 10. Click on accept SUCCESS => profile 2 now has the new value in the parameter 11. Select profile 2 and change the name to profile 1 SUCCESS => the page complains there is another profile with that name, and asks if you want to replace it 12. Click on accept SUCCESS => in profile select there is only one profile called profile 1 that has the values of profile 2 13. Select profile 1 and click remove profile SUCCESS => there is no profile in profile select. 14. Create a profile and click on “Stage for import” 15. Go to Staged MARC management page SUCCESS => Improt should have the name of the profile in profile column, and when you click on the file name, there should be the name of the profile in the details. 16. prove t/db_dependent/ImportBatch.t t/db_dependent/api/v1/import_batch_profiles.t 17. Sign off (In reply to Abbey Holt from comment #16) > This is great! There is one area where my matching preferences would not > stick when I created a profile. If I select "No" under "Check for embedded > item record data" and click "Add profile", No switches back to Yes. My > selection for "how to process items" is remembered. It is just the yes-no > button that isn't being saved. Hi Abbey, thanks so much for pointing that out.. could you please try again? Thanks! (In reply to Agustín Moyano from comment #18) > (In reply to Abbey Holt from comment #16) > > This is great! There is one area where my matching preferences would not > > stick when I created a profile. If I select "No" under "Check for embedded > > item record data" and click "Add profile", No switches back to Yes. My > > selection for "how to process items" is remembered. It is just the yes-no > > button that isn't being saved. > > Hi Abbey, thanks so much for pointing that out.. could you please try again? > > Thanks! It worked! I'm signing off. Created attachment 105555 [details] [review] Bug 23019: Add profiles to stage-import-batch and magnage-import-batch pages This patch adds the logic and the needed UI elements to be able to pre-load an import profile. It also displays which profile was used to stage an import in staged import manager. To test: 1. Apply all patches 2. Updatedatabase 3. Go to Stage MARC records for Import tool in admin, and upload a file with MARC records. CHECK => after uploading, there is a fieldset with the legend “Profile settings” => inside the fieldset there is a select labeled “Pre fill values with profile”. The only value it has is “Do not use profile”. 4. Change some settings, and set “profile 1” as profile name and click on “Add profile” SUCCESS => The select now has the new profile selected 5. Change profile select to “Do not use profile” SUCCESS => Default values are now displayed in the form 6. Reload the page and upload the file again SUCCESS => the select still has the profile recently added 7. Select the profile, change some parameter in the form and set the profile name to “profile 2”, and click add profile SUCCESS => there are two profiles now, and if you toggle between them, the parameter changes 8. Select profile 1, change one parameter and click on update profile SUCCESS => if you toggle that profile with the other, the new parameter of the value is shown when you select profile 1 9. Select profile 2, change some parameter and click Add profile (leaving the name as profile 2) SUCCESS => the page complains there is another profile with the same name, and asks if you want to replace it. 10. Click on accept SUCCESS => profile 2 now has the new value in the parameter 11. Select profile 2 and change the name to profile 1 SUCCESS => the page complains there is another profile with that name, and asks if you want to replace it 12. Click on accept SUCCESS => in profile select there is only one profile called profile 1 that has the values of profile 2 13. Select profile 1 and click remove profile SUCCESS => there is no profile in profile select. 14. Create a profile and click on “Stage for import” 15. Go to Staged MARC management page SUCCESS => Improt should have the name of the profile in profile column, and when you click on the file name, there should be the name of the profile in the details. 16. prove t/db_dependent/ImportBatch.t t/db_dependent/api/v1/import_batch_profiles.t 17. Sign off Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> (In reply to Abbey Holt from comment #16) > This is great! There is one area where my matching preferences would not > stick when I created a profile. If I select "No" under "Check for embedded > item record data" and click "Add profile", No switches back to Yes. My > selection for "how to process items" is remembered. It is just the yes-no > button that isn't being saved. Abbey, I think you need to acknowledge the attachments count on the bug, and tell the sandboxes that you want to SO 5 patches. Created attachment 105600 [details] [review] Bug 23019: Update database structure and add atomic update Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Created attachment 105601 [details] [review] Bug 23019: Add koha_object_class and koha_objects_class methods to ImportBatch.pm schema Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Created attachment 105602 [details] [review] Bug 23019: Add ImportBatchesProfile schema and set is_boolean to parse_items column Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Created attachment 105603 [details] [review] Bug 23019: Add tests Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Created attachment 105604 [details] [review] Bug 23019: Add profiles to stage-import-batch and magnage-import-batch pages This patch adds the logic and the needed UI elements to be able to pre-load an import profile. It also displays which profile was used to stage an import in staged import manager. To test: 1. Apply all patches 2. Updatedatabase 3. Go to Stage MARC records for Import tool in admin, and upload a file with MARC records. CHECK => after uploading, there is a fieldset with the legend “Profile settings” => inside the fieldset there is a select labeled “Pre fill values with profile”. The only value it has is “Do not use profile”. 4. Change some settings, and set “profile 1” as profile name and click on “Add profile” SUCCESS => The select now has the new profile selected 5. Change profile select to “Do not use profile” SUCCESS => Default values are now displayed in the form 6. Reload the page and upload the file again SUCCESS => the select still has the profile recently added 7. Select the profile, change some parameter in the form and set the profile name to “profile 2”, and click add profile SUCCESS => there are two profiles now, and if you toggle between them, the parameter changes 8. Select profile 1, change one parameter and click on update profile SUCCESS => if you toggle that profile with the other, the new parameter of the value is shown when you select profile 1 9. Select profile 2, change some parameter and click Add profile (leaving the name as profile 2) SUCCESS => the page complains there is another profile with the same name, and asks if you want to replace it. 10. Click on accept SUCCESS => profile 2 now has the new value in the parameter 11. Select profile 2 and change the name to profile 1 SUCCESS => the page complains there is another profile with that name, and asks if you want to replace it 12. Click on accept SUCCESS => in profile select there is only one profile called profile 1 that has the values of profile 2 13. Select profile 1 and click remove profile SUCCESS => there is no profile in profile select. 14. Create a profile and click on “Stage for import” 15. Go to Staged MARC management page SUCCESS => Improt should have the name of the profile in profile column, and when you click on the file name, there should be the name of the profile in the details. 16. prove t/db_dependent/ImportBatch.t t/db_dependent/api/v1/import_batch_profiles.t 17. Sign off Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> (In reply to Tomás Cohen Arazi from comment #21) > (In reply to Abbey Holt from comment #16) > > This is great! There is one area where my matching preferences would not > > stick when I created a profile. If I select "No" under "Check for embedded > > item record data" and click "Add profile", No switches back to Yes. My > > selection for "how to process items" is remembered. It is just the yes-no > > button that isn't being saved. > > Abbey, I think you need to acknowledge the attachments count on the bug, and > tell the sandboxes that you want to SO 5 patches. Thank you. I think I've signed off on everything now. Let me know if I need to do anything else! (In reply to Abbey Holt from comment #27) > (In reply to Tomás Cohen Arazi from comment #21) > > (In reply to Abbey Holt from comment #16) > > > This is great! There is one area where my matching preferences would not > > > stick when I created a profile. If I select "No" under "Check for embedded > > > item record data" and click "Add profile", No switches back to Yes. My > > > selection for "how to process items" is remembered. It is just the yes-no > > > button that isn't being saved. > > > > Abbey, I think you need to acknowledge the attachments count on the bug, and > > tell the sandboxes that you want to SO 5 patches. > > > Thank you. I think I've signed off on everything now. Let me know if I need > to do anything else! That worked great! This works, but I feel the GUI is not very intuitive. Actually, it took me a while to figure it out. The profile is "started" at the top of the page, but then after making your settings, you have to make sure to hit "Update profile" for the settings to be saved. So the workflow is like this to add a profile for it to work: 1) Enter a name for a profile, click on add 2) Make the settings you want 3) DONT CLICK IMPORT - or your profile will save with the wrong values 4) Go back up, click "Update profile" 5) Import I got it wrong the first time and was very confused why the "wrong" values showed up on managing the staged import. If a profile was selected, values can be changed. After importing it's not clear what was used for the import - the values from the profile or those set (and not saved in the profile) of the form? The profile values will show. I really believe we need to improve this a bit before including it. Ideas: - Remove the buttons from the profile on top of the form and only show the list of existing profiles there. - If a profile is selected, change the values in the form below. - When one of the settings is changed, show a hint behind the field. - When importing: - when profile is used and no field was changed: import - when no profile was used: import - when profile was used and values changed: ask if the profile or the changed values should be used (and possibly if the existing profile should be updated) - At the end of the form offer to save the settings as a new profile or update the existing one. This way, you can start at the top and work your way down. Alternatively: - Allow creating, updating and deleting profiles from a different configuration screen. - When a profile is selected, disable the other form fields so it's an "either or" and it's always clear what will be applied for the import. Created attachment 106555 [details] [review] Bug 23019: (QA follow-up) Fix typo Pre fill Oh also: Please write up the addition to the REST API: https://wiki.koha-community.org/wiki/REST_api_RFCs Hi Katrin, I'll respond between lines (In reply to Katrin Fischer from comment #29) > This works, but I feel the GUI is not very intuitive. Actually, it took me a > while to figure it out. > > The profile is "started" at the top of the page, but then after making your > settings, you have to make sure to hit "Update profile" for the settings to > be saved. So the workflow is like this to add a profile for it to work: > > 1) Enter a name for a profile, click on add > 2) Make the settings you want > 3) DONT CLICK IMPORT - or your profile will save with the wrong values > 4) Go back up, click "Update profile" > 5) Import > > I got it wrong the first time and was very confused why the "wrong" values > showed up on managing the staged import. > > If a profile was selected, values can be changed. After importing it's not > clear what was used for the import - the values from the profile or those > set (and not saved in the profile) of the form? The profile values will show. > Selecting a profile just pre-fills the form, if you change a value in the form, that's what counts for importing. I thought it good to keep that flexibility, thinking that maybe someone trying to import, might want to use almost all values from a profile, but change just one or two. > I really believe we need to improve this a bit before including it. > > Ideas: > > - Remove the buttons from the profile on top of the form and only show the > list of existing profiles there. > - If a profile is selected, change the values in the form below. > - When one of the settings is changed, show a hint behind the field. > - When importing: > - when profile is used and no field was changed: import > - when no profile was used: import > - when profile was used and values changed: ask if the profile or the > changed values should be used (and possibly if the existing profile should > be updated) > - At the end of the form offer to save the settings as a new profile or > update the existing one. This way, you can start at the top and work your > way down. > I'll remove the profile from the top and place them below. I'll also add a text to clarify that selecting a profile just pre-fill's your form, but values can be changed. > > Alternatively: > - Allow creating, updating and deleting profiles from a different > configuration screen. > - When a profile is selected, disable the other form fields so it's an > "either or" and it's always clear what will be applied for the import. I thought of this, but it would be a configuration screen exactly the same as this one.. it seemed kind of repetitive and less maintainable.
> I'll remove the profile from the top and place them below. I'll also add a
> text to clarify that selecting a profile just pre-fill's your form, but
> values can be changed.
>
I ment, I'll remove the profile buttons from the top
(In reply to Katrin Fischer from comment #31) > Oh also: Please write up the addition to the REST API: > https://wiki.koha-community.org/wiki/REST_api_RFCs Hi Katrin, here is the RFC https://wiki.koha-community.org/wiki/Import_batch_profiles_endpoint_RFC Is it ok?.. it's my first RFC.. Created attachment 109265 [details] [review] Bug 23019: (QA follow-up) Fix typo Pre fill Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 109266 [details] [review] Bug 23019: (follow-up) Fix usability issues This patch place profile buttons in the bottom of the page, next to "Stage for import" It removes the "Update profile" button, and changes "Add profile" to "Save profile". Save profile can be used to add and to update a profile. It also adds feedback when a profile was saved or deleted. Last, it also replaces input-batch-profile endpoint name for input_batch_profile, which is more aligned with other endpoint names. Ok, when I run qa I get this two errors: FAIL Koha/ImportBatch.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 which is weird, since Koha/ImportBatch.pm is created in this patchset FAIL api/v1/swagger/definitions/import-batch-profiles.json FAIL git manipulation The file has been added and deleted in the same patchset I get this error because I created api/v1/swagger/definitions/import-batch-profiles.json in a patch that is now signed off, and I'm trying to rename it to import_batch_profiles.json (Since previous patch is signed off, I don't think it wise to squash with a not signed patch).. because the file has less than 10 lines and file content also changes, git just marks it for delete, and creates the new file. This did not work in my testing, whenever I click 'Save profile' I get an error:
An error occurred
[object Object]
in console:
Uncaught (in promise)
{…}
abort: function abort(a)
always: function always()
complete: function add()
done: function add()
error: function add()
fail: function add()
getAllResponseHeaders: function getAllResponseHeaders()
getResponseHeader: function getResponseHeader(a)
overrideMimeType: function overrideMimeType(a)
pipe: function pipe()
progress: function add()
promise: function promise(a)
readyState: 4
responseText: "<!DOCTYPE html>\n<!-- Request ID: e249b4ad -->\n<html>\n <head>\n <title>Page not found (development mode)</title>\n <meta http-equiv=\"Pragma\" content=\"no-cache\">\n <meta http-equiv=\"Expires\" content=\"-1\">\n <script src=\"/mojo/jquery/jquery.js\"></script>\n <script src=\"/mojo/prettify/run_prettify.js\"></script>\n <link href=\"/mojo/prettify/prettify-mojo-dark.css\" rel=\"stylesheet\">\n <style>\n a img {\n border: 0;\n }\n body {\n background: url(/mojo/pinstripe-light.png);\n color: #445555;\n font: 0.9em 'Helvetica Neue', Helvetica, sans-serif;\n font-weight: normal;\n line-height: 1.5em;\n margin: 0;\n }\n :not(pre) > code {\n background-color: #eef9ff;\n border: solid #cce4ff 1px;\n border-radius: 5px;\n color: #333;\n font: 0.9em Consolas, Menlo, Monaco, Courier, monospace;\n padding: 0.4em;\n }\n h1 {\n color: #2a2a2a;\n font-size: 1.5em;\n margin: 0;…"
setRequestHeader: function setRequestHeader(a, b)
state: function state()
status: 404
statusCode: function statusCode(a)
statusText: "Not Found"
success: function add()
then: function then()
<prototype>: Object { … }
Created attachment 109637 [details] [review] Bug 23019: (follow-up) Fix usability issues This patch place profile buttons in the bottom of the page, next to "Stage for import" It removes the "Update profile" button, and changes "Add profile" to "Save profile". Save profile can be used to add and to update a profile. It also adds feedback when a profile was saved or deleted. Last, it also replaces input-batch-profile endpoint name for input_batch_profile, which is more aligned with other endpoint names. (In reply to Nick Clemens from comment #38) > This did not work in my testing, whenever I click 'Save profile' I get an > error: > An error occurred > > [object Object] > > in console: > Uncaught (in promise) > {…} > > abort: function abort(a) > > always: function always() > > complete: function add() > > done: function add() > > error: function add() > > fail: function add() > > getAllResponseHeaders: function getAllResponseHeaders() > > getResponseHeader: function getResponseHeader(a) > > overrideMimeType: function overrideMimeType(a) > > pipe: function pipe() > > progress: function add() > > promise: function promise(a) > > readyState: 4 > > responseText: "<!DOCTYPE html>\n<!-- Request ID: e249b4ad -->\n<html>\n > <head>\n <title>Page not found (development mode)</title>\n <meta > http-equiv=\"Pragma\" content=\"no-cache\">\n <meta > http-equiv=\"Expires\" content=\"-1\">\n <script > src=\"/mojo/jquery/jquery.js\"></script>\n <script > src=\"/mojo/prettify/run_prettify.js\"></script>\n <link > href=\"/mojo/prettify/prettify-mojo-dark.css\" rel=\"stylesheet\">\n > <style>\n a img {\n border: 0;\n }\n body {\n > background: url(/mojo/pinstripe-light.png);\n color: #445555;\n > font: 0.9em 'Helvetica Neue', Helvetica, sans-serif;\n font-weight: > normal;\n line-height: 1.5em;\n margin: 0;\n }\n > :not(pre) > code {\n background-color: #eef9ff;\n border: > solid #cce4ff 1px;\n border-radius: 5px;\n color: #333;\n > font: 0.9em Consolas, Menlo, Monaco, Courier, monospace;\n padding: > 0.4em;\n }\n h1 {\n color: #2a2a2a;\n font-size: > 1.5em;\n margin: 0;…" > > setRequestHeader: function setRequestHeader(a, b) > > state: function state() > > status: 404 > > statusCode: function statusCode(a) > > statusText: "Not Found" > > success: function add() > > then: function then() > > <prototype>: Object { … } Hi Nick, sorry about that.. forgot to change where endpoint was called. Please try again Thanks! Several tests are failing:
# Failed test 'exact match for JSON Pointer "/name"'
# at t/db_dependent/api/v1/import_batch_profiles.t line 134.
# got: undef
# expected: 'theProfile'
# Failed test 'profile name should be the updated one'
# at t/db_dependent/api/v1/import_batch_profiles.t line 138.
# got: 'someProfile'
# expected: 'theProfile'
# Looks like you failed 4 tests of 5.
t/db_dependent/api/v1/import_batch_profiles.t .. 3/4
# Failed test 'edit profile'
# at t/db_dependent/api/v1/import_batch_profiles.t line 143.
# Failed test '204 No Content'
# at t/db_dependent/api/v1/import_batch_profiles.t line 168.
# got: '404'
# expected: '204'
# Failed test 'profile should be erased'
# at t/db_dependent/api/v1/import_batch_profiles.t line 173.
# got: 'Koha::ImportBatchProfile=HASH(0x55d394fd0cb8)'
# expected: undef
# Looks like you failed 2 tests of 3.
Created attachment 112531 [details] [review] Bug 23019: Update database structure and add atomic update Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Created attachment 112532 [details] [review] Bug 23019: Add koha_object_class and koha_objects_class methods to ImportBatch.pm schema Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Created attachment 112533 [details] [review] Bug 23019: Add ImportBatchesProfile schema and set is_boolean to parse_items column Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Created attachment 112534 [details] [review] Bug 23019: Add tests Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Created attachment 112535 [details] [review] Bug 23019: Add profiles to stage-import-batch and magnage-import-batch pages This patch adds the logic and the needed UI elements to be able to pre-load an import profile. It also displays which profile was used to stage an import in staged import manager. To test: 1. Apply all patches 2. Updatedatabase 3. Go to Stage MARC records for Import tool in admin, and upload a file with MARC records. CHECK => after uploading, there is a fieldset with the legend “Profile settings” => inside the fieldset there is a select labeled “Pre fill values with profile”. The only value it has is “Do not use profile”. 4. Change some settings, and set “profile 1” as profile name and click on “Add profile” SUCCESS => The select now has the new profile selected 5. Change profile select to “Do not use profile” SUCCESS => Default values are now displayed in the form 6. Reload the page and upload the file again SUCCESS => the select still has the profile recently added 7. Select the profile, change some parameter in the form and set the profile name to “profile 2”, and click add profile SUCCESS => there are two profiles now, and if you toggle between them, the parameter changes 8. Select profile 1, change one parameter and click on update profile SUCCESS => if you toggle that profile with the other, the new parameter of the value is shown when you select profile 1 9. Select profile 2, change some parameter and click Add profile (leaving the name as profile 2) SUCCESS => the page complains there is another profile with the same name, and asks if you want to replace it. 10. Click on accept SUCCESS => profile 2 now has the new value in the parameter 11. Select profile 2 and change the name to profile 1 SUCCESS => the page complains there is another profile with that name, and asks if you want to replace it 12. Click on accept SUCCESS => in profile select there is only one profile called profile 1 that has the values of profile 2 13. Select profile 1 and click remove profile SUCCESS => there is no profile in profile select. 14. Create a profile and click on “Stage for import” 15. Go to Staged MARC management page SUCCESS => Improt should have the name of the profile in profile column, and when you click on the file name, there should be the name of the profile in the details. 16. prove t/db_dependent/ImportBatch.t t/db_dependent/api/v1/import_batch_profiles.t 17. Sign off Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Created attachment 112536 [details] [review] Bug 23019: (QA follow-up) Fix typo Pre fill Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 112537 [details] [review] Bug 23019: (follow-up) Fix usability issues This patch place profile buttons in the bottom of the page, next to "Stage for import" It removes the "Update profile" button, and changes "Add profile" to "Save profile". Save profile can be used to add and to update a profile. It also adds feedback when a profile was saved or deleted. Last, it also replaces input-batch-profile endpoint name for input_batch_profile, which is more aligned with other endpoint names. (In reply to Katrin Fischer from comment #41) > Several tests are failing: > > # Failed test 'exact match for JSON Pointer "/name"' > # at t/db_dependent/api/v1/import_batch_profiles.t line 134. > # got: undef > # expected: 'theProfile' > > # Failed test 'profile name should be the updated one' > # at t/db_dependent/api/v1/import_batch_profiles.t line 138. > # got: 'someProfile' > # expected: 'theProfile' > # Looks like you failed 4 tests of 5. > t/db_dependent/api/v1/import_batch_profiles.t .. 3/4 > # Failed test 'edit profile' > # at t/db_dependent/api/v1/import_batch_profiles.t line 143. > > # Failed test '204 No Content' > # at t/db_dependent/api/v1/import_batch_profiles.t line 168. > # got: '404' > # expected: '204' > > # Failed test 'profile should be erased' > # at t/db_dependent/api/v1/import_batch_profiles.t line 173. > # got: 'Koha::ImportBatchProfile=HASH(0x55d394fd0cb8)' > # expected: undef > # Looks like you failed 2 tests of 3. Hi Katrin, could you please try again? Thanks Created attachment 112715 [details] [review] Bug 23019: Update database structure and add atomic update Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 112716 [details] [review] Bug 23019: Add koha_object_class and koha_objects_class methods to ImportBatch.pm schema Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 112717 [details] [review] Bug 23019: Add ImportBatchesProfile schema and set is_boolean to parse_items column Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 112718 [details] [review] Bug 23019: Add tests Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 112719 [details] [review] Bug 23019: Add profiles to stage-import-batch and magnage-import-batch pages This patch adds the logic and the needed UI elements to be able to pre-load an import profile. It also displays which profile was used to stage an import in staged import manager. To test: 1. Apply all patches 2. Updatedatabase 3. Go to Stage MARC records for Import tool in admin, and upload a file with MARC records. CHECK => after uploading, there is a fieldset with the legend “Profile settings” => inside the fieldset there is a select labeled “Pre fill values with profile”. The only value it has is “Do not use profile”. 4. Change some settings, and set “profile 1” as profile name and click on “Add profile” SUCCESS => The select now has the new profile selected 5. Change profile select to “Do not use profile” SUCCESS => Default values are now displayed in the form 6. Reload the page and upload the file again SUCCESS => the select still has the profile recently added 7. Select the profile, change some parameter in the form and set the profile name to “profile 2”, and click add profile SUCCESS => there are two profiles now, and if you toggle between them, the parameter changes 8. Select profile 1, change one parameter and click on update profile SUCCESS => if you toggle that profile with the other, the new parameter of the value is shown when you select profile 1 9. Select profile 2, change some parameter and click Add profile (leaving the name as profile 2) SUCCESS => the page complains there is another profile with the same name, and asks if you want to replace it. 10. Click on accept SUCCESS => profile 2 now has the new value in the parameter 11. Select profile 2 and change the name to profile 1 SUCCESS => the page complains there is another profile with that name, and asks if you want to replace it 12. Click on accept SUCCESS => in profile select there is only one profile called profile 1 that has the values of profile 2 13. Select profile 1 and click remove profile SUCCESS => there is no profile in profile select. 14. Create a profile and click on “Stage for import” 15. Go to Staged MARC management page SUCCESS => Improt should have the name of the profile in profile column, and when you click on the file name, there should be the name of the profile in the details. 16. prove t/db_dependent/ImportBatch.t t/db_dependent/api/v1/import_batch_profiles.t 17. Sign off Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Signed-off-by: Abbey Holt <aholt@dubuque.lib.ia.us> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 112720 [details] [review] Bug 23019: (QA follow-up) Fix typo Pre fill Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 112721 [details] [review] Bug 23019: (follow-up) Fix usability issues This patch place profile buttons in the bottom of the page, next to "Stage for import" It removes the "Update profile" button, and changes "Add profile" to "Save profile". Save profile can be used to add and to update a profile. It also adds feedback when a profile was saved or deleted. Last, it also replaces input-batch-profile endpoint name for input_batch_profile, which is more aligned with other endpoint names. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 112722 [details] [review] Bug 23019: (QA follow-up) fix test fiule permissions Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Shouldn't the table be named "import_batch_profiles" instead of "import_batches_profile"? 1. Shouldn't the table be named "import_batch_profiles" instead of "import_batches_profile"?
2. Double check string escaping (save profile with "<alert>script("❤");</alert>" for instance)
3.
+ "x-koha-authorization": {
+ "permissions": {
+ "catalogue": "1"
+ }
+ }
Is this correct? Should not it be { tools => 'stage_marc_import' } like tools/stage-marc-import.pl instead?
4. In api/v1/swagger/parameters.json
+ "profile_id_pp": {
should not it be more specific, "import_batch_profile_id"?
5. Error handling: Insert a long name for the profile: """ An error occurred [object Object] """ Created attachment 113276 [details] [review] Bug 23019: (follow-up) set table name to import_batch_profiles Created attachment 113277 [details] [review] Bug 23019: (follow-up) Fix other things This patch: * Fixes table name change * Adds schema file * Changes api parameter name * Fixes string escaping * Fixes error handling Created attachment 113311 [details] [review] Bug 23019: (follow-up 2) set table name to import_batch_profiles t/db_dependent/api/v1/import_batch_profiles.t is failing. Created attachment 113416 [details] [review] Bug 23019: (follow-up) Fix test permissions and API definition bugs (In reply to Agustín Moyano from comment #65) > Created attachment 113416 [details] [review] [review] > Bug 23019: (follow-up) Fix test permissions and API definition bugs I don't understand why you need to adjust the permissions in tests, flags => 1 means superlibrarian. (In reply to Jonathan Druart from comment #66) > (In reply to Agustín Moyano from comment #65) > > Created attachment 113416 [details] [review] [review] [review] > > Bug 23019: (follow-up) Fix test permissions and API definition bugs > > I don't understand why you need to adjust the permissions in tests, flags => > 1 means superlibrarian. Ok got it, the tests pass without it. (In reply to Jonathan Druart from comment #67) > (In reply to Jonathan Druart from comment #66) > > (In reply to Agustín Moyano from comment #65) > > > Created attachment 113416 [details] [review] [review] [review] [review] > > > Bug 23019: (follow-up) Fix test permissions and API definition bugs > > > > I don't understand why you need to adjust the permissions in tests, flags => > > 1 means superlibrarian. > > Ok got it, the tests pass without it. No, I really got it all wrong about permissions, hehe.. I'll attach an improved test. Created attachment 113421 [details] [review] Bug 23019: (follow-up) Fix test permissions and API definition bugs Pushed to master for 20.11, thanks to everybody involved! new feature will not be backported to 20.05.x |