|
Description
Lisette Scheer
2019-08-02 15:35:03 UTC
Created attachment 107662 [details] [review] Bug 23420: Add OPACSuggestionUnwantedFields option To test: -Apply patch -Check OPACSuggestionUnwantedFields -Check some fields as unwanted -Go to the OPAC suggestion page and make sure these correct fields are hidden -Go through each option and make sure it hides properly Created attachment 107787 [details] [review] Bug 23420: Add OPACSuggestionUnwantedFields option To test: -Apply patch -Check OPACSuggestionUnwantedFields -Check some fields as unwanted -Go to the OPAC suggestion page and make sure these correct fields are hidden -Go through each option and make sure it hides properly Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Worked great! Really like this, but 2 problems: - patch set is missing the change to sysprefs.sql - after updatedatabase is run, the title is hidden by default. The statement needs to be corrected. Created attachment 108426 [details] [review] Bug 23420: Add OPACSuggestionUnwantedFields option To test: -Apply patch -Check OPACSuggestionUnwantedFields -Check some fields as unwanted -Go to the OPAC suggestion page and make sure these correct fields are hidden -Go through each option and make sure it hides properly Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Thanks Cait, I added a new patch, kept Lisette's sign-off, but reset the status to 'Needs Signoff'. Not sure what's appropriate here. Usually the best way is adding a second patch was (QA follow-up) - then you can also switch back to signed off, because it makes it easy to see the additional changes. If there are too many, QA could send it back to Needs signoff, but in this case it would be fine. It also means the sign-off remains 'untouched' as the signed patch hasn't changed. (hope that makes sense, it's late!) Created attachment 108429 [details] [review] Bug 23420: Add OPACSuggestionUnwantedFields option To test: -Apply patch -Check OPACSuggestionUnwantedFields -Check some fields as unwanted -Go to the OPAC suggestion page and make sure these correct fields are hidden -Go through each option and make sure it hides properly Created attachment 108430 [details] [review] Bug 23420: (QA follow-up) fix DB update and add syspref.sql Also added a note with the sys pref to remind not to makeOPACSuggestionMandatoryFields unwanted Probably this is a separate bug but OPACSuggestionMandatoryFields doesn't seem to have a spot in syspref.sql Title is a mandatory field even if it isn't selected by OPACSuggestionMandatoryFields. I think that means it should be excluded from this preference. Created attachment 109299 [details] [review] Bug 23420: (follow-up) remove title this no longer clean applies, will rebase Created attachment 110414 [details] [review] Bug 23420: Add OPACSuggestionUnwantedFields system pref To test: -Apply patch -Check OPACSuggestionUnwantedFields -Check some fields as unwanted -Go to the OPAC suggestion page and make sure these correct fields are hidden -Go through each option and make sure it hides properly The 'Library or branch' field doesn't seem to display at all. If no unwanted fields are selected, it doesn't display. When just it is unselected, it doesn't display either. All other fields can be displayed or not displayed. (In reply to David Nind from comment #16) > The 'Library or branch' field doesn't seem to display at all. > > If no unwanted fields are selected, it doesn't display. > > When just it is unselected, it doesn't display either. > > All other fields can be displayed or not displayed. Hi David, Thanks for testing. The branch/library selection field will not show on the purchase suggestions form unless the system preference 'AllowPurchaseSuggestionBranchChoice' is set to allow. Also worth noting, the 'Patron Reason' field will only display when there are authorized values in the OPAC_SUG category. Created attachment 110418 [details] [review] Bug 23420: Add OPACSuggestionUnwantedFields system pref To test: -Apply patch -Check OPACSuggestionUnwantedFields -Check some fields as unwanted -Go to the OPAC suggestion page and make sure these correct fields are hidden -Turn on the system preference AllowPurchaseSuggestionBranchChoice to see the branch choice on the form -Make sure you can correctly hide this one and still submit the purchase suggestion for without error. -Go through each option and make sure it hides properly. Note: You will only see the patron reason field when there are authorised values in the OPAC_SUG category I submitted a new patch with an updated test plan so I am switching this back to Needs Sign-off Created attachment 110419 [details] [review] Bug 23420: Add OPACSuggestionUnwantedFields system pref To test: -Apply patch -Check OPACSuggestionUnwantedFields -Check some fields as unwanted -Go to the OPAC suggestion page and make sure these correct fields are hidden -Turn on the system preference AllowPurchaseSuggestionBranchChoice to see the branch choice on the form -Make sure you can correctly hide this one and still submit the purchase suggestion for without error. -Go through each option and make sure it hides properly. Note: You will only see the patron reason field when there are authorised values in the OPAC_SUG category Signed-off-by: David Nind <david@davidnind.com> Thanks Lucas! I have retested and signed off. Everything works as expected now with AllowPurchaseSuggestionBranchChoice enabled. David Hi Lucas, I feel like having 2 switches for the same thing (visibility of library ) can be confusing. Is there a reason we should not take the chance here to remove AllowPurchaseSuggestionBranchChoice in favor of the new pref? We could add a line to the databaseupdate to make the branch invisible if AllowPurchaseSuggestionBranchChoice is set to don't allow now. Created attachment 110671 [details] [review] Bug 23420: (QA follow-up) Remove AllowPurchaseSuggestionBranchChoice syspref Created attachment 110688 [details] [review] Bug 23420: Add OPACSuggestionUnwantedFields system pref To test: -Apply patch -Check OPACSuggestionUnwantedFields -Check some fields as unwanted -Go to the OPAC suggestion page and make sure these correct fields are hidden -Turn on the system preference AllowPurchaseSuggestionBranchChoice to see the branch choice on the form -Make sure you can correctly hide this one and still submit the purchase suggestion for without error. -Go through each option and make sure it hides properly. Note: You will only see the patron reason field when there are authorised values in the OPAC_SUG category Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 110689 [details] [review] Bug 23420: (QA follow-up) Remove AllowPurchaseSuggestionBranchChoice syspref Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Thx for the follow-up, Lucas! The pref is still used
% git grep -i AllowPurchaseSuggestionBranchChoice **/*.pm **/*.pl **/*.t
C4/UsageStats.pm: AllowPurchaseSuggestionBranchChoice
installer/data/mysql/updatedatabase.pl: $dbh->do("INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES ('AllowPurchaseSuggestionBranchChoice', 0, 'Allow user to choose branch when making a purchase suggestion','1','YesNo')");
opac/opac-suggestions.pl:if ( C4::Context->preference("AllowPurchaseSuggestionBranchChoice") ) {
t/db_dependent/UsageStats.t: AllowPurchaseSuggestionBranchChoice
Created attachment 111254 [details] [review] Bug 23420: (QA follow-up) Remove more AllowPurchaseSuggestionBranchChoice code Test plan: 1. git grep -i AllowPurchaseSuggestionBranchChoice **/*.pm **/*.pl **/*.t 2. There should no longer be any code related to the old sys pref AllowPurchaseSuggestionBranchChoice Pushed to master for 20.11, thanks to everybody involved! enhancement will not be backported to 20.05.x |