Description
Kyle M Hall
2012-12-11 14:43:21 UTC
Created attachment 13999 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons This patch adds a new tab to the patron side menu for purchase suggestions. From this new tab, a librarian can view the patron's existing purchase suggestios and also create new suggestions in the name of that patron. Test Plan: 1) Apply patch 2) Ensure the system preference 'suggestions' is enabled 3) View the details for a patron 4) Click the new 'Purchase suggestions' tab 5) Click the 'New purchase suggestion' button 6) Add the new purchase suggestions 7) You should now end up back at the borrowers purchase suggetions 8) Verify the new purchase suggestion was added *** Bug 8527 has been marked as a duplicate of this bug. *** Created attachment 14612 [details] [review] Verification of Previous Patch. OK. QA comments: The patch is clean and works well. But I have some remarks: - template_name => "members/purchase-suggestions.tmpl" could be .tt - in suggestion.pl, %hashlists is passed twice. - in suggestion.pl, $redirect could be eq to "purcharse-suggestion" with a new param borrowernumber=[%borrowernumber%] in order to open the possibility for an other script to use this redirection. - in purchase-suggestion.tt, some includes are useless: calendar.inc, tablesorter.js - The new tab is sometimes displayed on 2 lines and sometimes on 1 lines depending on which tab is enabled. I don't know how to fix that... - on the way, could you fix 4 withespaces: * suggestion/suggestion.pl FAIL forbidden patterns FAIL The patch introduces a forbidden pattern: withespace character (77) The patch introduces a forbidden pattern: withespace character (138) * koha-tmpl/intranet-tmpl/prog/en/modules/members/purchase-suggestions.tt FAIL forbidden patterns FAIL The patch introduces a forbidden pattern: withespace character (71) The patch introduces a forbidden pattern: withespace character (62) Marked as Failed QA Created attachment 14675 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup Created attachment 14677 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup > - template_name => "members/purchase-suggestions.tmpl" could be .tt Fixed! > - in suggestion.pl, %hashlists is passed twice. Fixed! > - in suggestion.pl, $redirect could be eq to "purcharse-suggestion" with a > new param borrowernumber=[%borrowernumber%] in order to open the possibility > for an other script to use this redirection. Fixed! > - in purchase-suggestion.tt, some includes are useless: calendar.inc, > tablesorter.js Fixed! > - The new tab is sometimes displayed on 2 lines and sometimes on 1 lines > depending on which tab is enabled. I don't know how to fix that... This is because the tabs are not the same width on every page. Sometimes they are wide enough to get both words on one line, sometimes they are not. I've fixed this by forcing each word to always be on a separate line. > - on the way, could you fix 4 withespaces: > * suggestion/suggestion.pl > FAIL > forbidden patterns FAIL > The patch introduces a forbidden pattern: withespace character (77) > The patch introduces a forbidden pattern: withespace character (138) > * koha-tmpl/intranet-tmpl/prog/en/modules/members/purchase-suggestions.tt > FAIL > forbidden patterns FAIL > The patch introduces a forbidden pattern: withespace character (71) > The patch introduces a forbidden pattern: withespace character (62) > Fixed! Could you rebase once again please (and perhaps squash these 2 patch into 1) ? Created attachment 16602 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons This patch adds a new tab to the patron side menu for purchase suggestions. From this new tab, a librarian can view the patron's existing purchase suggestios and also create new suggestions in the name of that patron. Test Plan: 1) Apply patch 2) Ensure the system preference 'suggestions' is enabled 3) View the details for a patron 4) Click the new 'Purchase suggestions' tab 5) Click the 'New purchase suggestion' button 6) Add the new purchase suggestions 7) You should now end up back at the borrowers purchase suggetions 8) Verify the new purchase suggestion was added Signed-off-by: Corey Fuimaono <agent.075@gmail.com> Step though the test plan. All OK. Created attachment 16603 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup *** Bug 9777 has been marked as a duplicate of this bug. *** I got Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm line 2054. when clicking on the "New purchase suggestion" button (empty list) Created attachment 20002 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons This patch adds a new tab to the patron side menu for purchase suggestions. From this new tab, a librarian can view the patron's existing purchase suggestios and also create new suggestions in the name of that patron. Test Plan: 1) Apply patch 2) Ensure the system preference 'suggestions' is enabled 3) View the details for a patron 4) Click the new 'Purchase suggestions' tab 5) Click the 'New purchase suggestion' button 6) Add the new purchase suggestions 7) You should now end up back at the borrowers purchase suggetions 8) Verify the new purchase suggestion was added Signed-off-by: Corey Fuimaono <agent.075@gmail.com> Step though the test plan. All OK. Created attachment 20003 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup A couple of issues: - The template tries to build an old YUI-style button instead of the current Bootstrap version. - The table of suggestions should link to the suggestions detail page for that entry if the user has the correct permissions. (/cgi-bin/koha/suggestion/suggestion.pl?suggestionid=X&op=show I assume, a page which should probably be made more useful somehow). Created attachment 20007 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup 2 Created attachment 20008 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup 2 This latest patch doesn't remove the YUI JavaScript, so even though the button looks correct there is still a JS error on the page. Created attachment 20806 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup 3 Hi Kyle, starting with the tests and a first code review: Would you be ok updating to GPLv3+? FAIL members/purchase-suggestions.pl OK pod FAIL forbidden patterns forbidden pattern: Koha is now under the GPLv3 license (line 9) Is this still relevant? + DHTMLcalendar_dateformat => C4::Dates->DHTMLcalendar(), I wonder if we need a way to allow for disabling this functionality in the patron account or a way to differentiate between suggestions created by the user in the OPAC and by a staff member. And not failing for this - but so you can take a look at the other 2 things. Hi Kyle, this patch would be very interesting for us. Do you think you can fix the issues raised by Katrin? For the idea of making a distinction between 2 kinds of suggestion, I don't think it is necessary. If a patron fill a printed suggestion form and if we copy it in Koha (that's the way we could use it, to manage all our suggestions in Koha), he won't be surprised to see this suggestion in OPAC. M. Saby Created attachment 22843 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup 4 QA comment: All works as expected, no bug found. Maybe the redirect stuff could be used when clicking on a suggestion from the patron page (with a link "back to the patron's suggestions", instead of "Back to suggestions"). But it is not blocker. 2 errors from qa tools: FAIL members/purchase-suggestions.pl FAIL forbidden patterns forbidden pattern: Koha is now under the GPLv3 license (line 9) FAIL koha-tmpl/intranet-tmpl/prog/en/modules/members/purchase-suggestions.tt FAIL forbidden patterns forbidden pattern: To include datatables files, please use the include file (see bug 10868) (line 11) Will be fixed in a followup. Please Kyle protests if you disagree with the GPL version change (I only write that for the sake of form, I know you will agree :)). Marked as Passed QA. Created attachment 22897 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons This patch adds a new tab to the patron side menu for purchase suggestions. From this new tab, a librarian can view the patron's existing purchase suggestios and also create new suggestions in the name of that patron. Test Plan: 1) Apply patch 2) Ensure the system preference 'suggestions' is enabled 3) View the details for a patron 4) Click the new 'Purchase suggestions' tab 5) Click the 'New purchase suggestion' button 6) Add the new purchase suggestions 7) You should now end up back at the borrowers purchase suggetions 8) Verify the new purchase suggestion was added Signed-off-by: Corey Fuimaono <agent.075@gmail.com> Step though the test plan. All OK. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 22898 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 22899 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup 2 Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 22900 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup 3 Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 22901 [details] [review] Bug 9261 - Allow librarians to make purchase suggestions for patrons - QA Followup 4 Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 22902 [details] [review] Bug 9261: QA followup: ergonomic improvement and GPL version change This patch: - changes the GPL version from 2 to 3. - uses the datatables.inc file - removes the single pixel before and after the DT (modifying the sDom value). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 22903 [details] [review] Bug 9261: Title is required when adding a new suggestion I don't know if it was a bug but at the OPAC, title is a required field and not on the staff interface. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Pushed to master. Thanks, Kyle! |