Summary: | Warns when exporting a basket | ||
---|---|---|---|
Product: | Koha | Reporter: | Aleisha Amohia <aleisha> |
Component: | Acquisitions | Assignee: | Aleisha Amohia <aleisha> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | minor | ||
Priority: | P5 - low | CC: | alex.arnaud, fridolin.somers, m.de.rooy, nick, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | Sponsored | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | 8612 | ||
Bug Blocks: | |||
Attachments: |
Bug 19200: Preventing warns when exporting a basket
Bug 19200: Preventing warns when exporting a basket Bug 19200: Preventing warns when exporting a basket Bug 19200: Preventing warns when exporting a basket Bug 19200: (QA follow-up) Simplify call to GetBasketAsCSV |
Description
Aleisha Amohia
2017-08-29 21:07:05 UTC
Created attachment 66602 [details] [review] Bug 19200: Preventing warns when exporting a basket To test: 1) Go to Tools -> CSV profiles -> New CSV Profile 2) Create a new CSV profile with any name of SQL fields. Ensure profile type: SQL and usage: basket export in acquisition 3) Go to Acquisitions -> Find or create a vendor -> Use or create a basket 4) Click the dropdown menu next to the 'Export as CSV' button. There should be a 'Default' option and your new CSV profile (at least) 5) Click the 'Default' option. Notice warns 6) Click the 'Export as CSV' button. Notice warns 7) Click your new CSV profile option. Notice warns 8) Apply patch and refresh page 9) Repeat steps 5-7, confirm that warns do not show 10) Confirm export still works as expected Sponsored-by: Catalyst IT Created attachment 67799 [details] [review] Bug 19200: Preventing warns when exporting a basket To test: 1) Go to Tools -> CSV profiles -> New CSV Profile 2) Create a new CSV profile with any name of SQL fields. Ensure profile type: SQL and usage: basket export in acquisition 3) Go to Acquisitions -> Find or create a vendor -> Use or create a basket 4) Click the dropdown menu next to the 'Export as CSV' button. There should be a 'Default' option and your new CSV profile (at least) 5) Click the 'Default' option. Notice warns 6) Click the 'Export as CSV' button. Notice warns 7) Click your new CSV profile option. Notice warns 8) Apply patch and refresh page 9) Repeat steps 5-7, confirm that warns do not show 10) Confirm export still works as expected Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> This looks like a change in behavior. When the cgi param is default, no profile id was passed. I guess what you wanted to do was just if( $param && $param eq ... ) ? (In reply to Marcel de Rooy from comment #3) > This looks like a change in behavior. When the cgi param is default, no > profile id was passed. > I guess what you wanted to do was just if( $param && $param eq ... ) ? Hi Marcel, there is no change in behaviour in my patch. It looks like there's originally a bug here, because the original code says "if $query->param('csv_profile') eq 'default'", but the word 'default' is never passed through that param, based on acqui/basket.tt. this means that when the Default option was selected, the $query->param('csv_profile') is not actually set to 'default' so an uninitialised value is sent to the GetBasketAsCSV method and the 'if' condition is never true. (I hope that makes sense. If you put a warn inside the if statement under where $query->param('csv_profile') eq default, you'll see that it never enters here and never prints the warn.) my patch checks if the $query->param('csv_profile') has been defined. if it has, then it uses one of the library-made csv profiles. if it hasn't been defined then it must be using the default profile and no ID is passed, as intended, with no warns. setting back to needs signoff as I believe this is still a valid patch. Created attachment 69959 [details] [review] Bug 19200: Preventing warns when exporting a basket To test: 1) Go to Tools -> CSV profiles -> New CSV Profile 2) Create a new CSV profile with any name of SQL fields. Ensure profile type: SQL and usage: basket export in acquisition 3) Go to Acquisitions -> Find or create a vendor -> Use or create a basket 4) Click the dropdown menu next to the 'Export as CSV' button. There should be a 'Default' option and your new CSV profile (at least) 5) Click the 'Default' option. Notice warns 6) Click the 'Export as CSV' button. Notice warns 7) Click your new CSV profile option. Notice warns 8) Apply patch and refresh page 9) Repeat steps 5-7, confirm that warns do not show 10) Confirm export still works as expected Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk> Is the if() else() statement still usefull? Why not just one call to GetBasketAsCSV? QA: Resuming here (In reply to Aleisha Amohia from comment #4) > (In reply to Marcel de Rooy from comment #3) > > This looks like a change in behavior. When the cgi param is default, no > > profile id was passed. > > I guess what you wanted to do was just if( $param && $param eq ... ) ? > > Hi Marcel, > > there is no change in behaviour in my patch. It looks like there's > originally a bug here, because the original code says "if > $query->param('csv_profile') eq 'default'", but the word 'default' is never > passed through that param, based on acqui/basket.tt. this means that when > the Default option was selected, the $query->param('csv_profile') is not > actually set to 'default' so an uninitialised value is sent to the > GetBasketAsCSV method and the 'if' condition is never true. (I hope that > makes sense. If you put a warn inside the if statement under where > $query->param('csv_profile') eq default, you'll see that it never enters > here and never prints the warn.) > > my patch checks if the $query->param('csv_profile') has been defined. if it > has, then it uses one of the library-made csv profiles. if it hasn't been > defined then it must be using the default profile and no ID is passed, as > intended, with no warns. > > setting back to needs signoff as I believe this is still a valid patch. You are right that the compare with 'default' makes no sense. It may have been a leftover from the past. (In reply to Alex Arnaud from comment #6) > Is the if() else() statement still usefull? Why not just one call to > GetBasketAsCSV? Yes. Adding a follow-up for that. Created attachment 70113 [details] [review] Bug 19200: Preventing warns when exporting a basket To test: 1) Go to Tools -> CSV profiles -> New CSV Profile 2) Create a new CSV profile with any name of SQL fields. Ensure profile type: SQL and usage: basket export in acquisition 3) Go to Acquisitions -> Find or create a vendor -> Use or create a basket 4) Click the dropdown menu next to the 'Export as CSV' button. There should be a 'Default' option and your new CSV profile (at least) 5) Click the 'Default' option. Notice warns 6) Click the 'Export as CSV' button. Notice warns 7) Click your new CSV profile option. Notice warns 8) Apply patch and refresh page 9) Repeat steps 5-7, confirm that warns do not show 10) Confirm export still works as expected Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 70114 [details] [review] Bug 19200: (QA follow-up) Simplify call to GetBasketAsCSV If no profile_id is passed, GetBasketAsCSV will fallback to default itself. No need to make the distinction in basket.pl. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed to master for 18.05, thanks to everybody involved! Awesome work all, pushed to stable for 17.11.02 Depends on Bug 8612 that is not in 17.05.x |