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.

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