Bug 12702 - Suggestions still accessible in OPAC when deactivated
Summary: Suggestions still accessible in OPAC when deactivated
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL: /cgi-bin/koha/opac-suggestions.pl
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-02 21:57 UTC by Katrin Fischer
Modified: 2016-12-05 21:23 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12702 - Suggestions still accessible in OPAC when deactivated (993 bytes, patch)
2015-01-13 23:29 UTC, Thomas Wright
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12702 - Suggestions still accessible in OPAC when deactivated (1.03 KB, patch)
2015-01-14 08:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 12702 - Suggestions still accessible in OPAC when deactivated (1.10 KB, patch)
2015-01-15 06:14 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2014-08-02 21:57:48 UTC
To reproduce:

- Turn off suggestion and AnonSuggestions.
- Verify there are no longer links visible in navigation and patron account
  to visit the OPAC suggestion page.
- Try to access the page directly: .../cgi-bin/koha/opac-suggestions.pl
- Verify that the page is still shown.
Comment 1 Marc Véron 2015-01-05 15:44:09 UTC
Confirmed.

With both Sysprefs AnonSuggestions and suggestion turned off, I was redirected to the login page and after login the page "Your purchase suggestions" displayed. I could even put a suggestion.
Comment 2 Marc Véron 2015-01-05 15:54:05 UTC
Pattern for solution in Bug 10595
Comment 3 Marc Véron 2015-01-05 16:01:55 UTC
Maybe something for the Catalyst Open Source Academy... (Starting today)
Comment 4 Thomas Wright 2015-01-13 20:47:01 UTC
I'm working on this.
Comment 5 Thomas Wright 2015-01-13 23:29:33 UTC Comment hidden (obsolete)
Comment 6 Thomas Wright 2015-01-14 01:19:13 UTC
Test Plan:

-Set preference 'suggestion' to 'Don't Allow'.
Navigate to .../cgi-bin/koha/opac-suggestions.pl
*You should be presented with a 404 error. This is the case even if 'AnonSuggestions' are allowed. 

I am assuming that if suggestions by patrons are not wanted that AnonSuggestions are also not wanted. I think this is a reasonable assumption.

-Set preference 'suggestion' to 'Allow' and set preference 'AnonSuggestions' to 'Don't Allow'
Navigate to .../cgi-bin/koha/opac-suggestions.pl from a non logged in user page.
*You should be presented with a login screen.
Log in using that page and you should be presented with the normal suggestions page.

-Set preference 'suggestion' to 'Allow' and set preference 'AnonSuggestions' to 'Allow'
Navigate to .../cgi-bin/koha/opac-suggestions.pl from a non logged in user page.
*You should be presented with the normal suggestions page.
Log in and navigate to .../cgi-bin/koha/opac-suggestions.pl
*You should be presented with the normal suggestions page.
Comment 7 Martin Renvoize 2015-01-14 08:41:04 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize 2015-01-14 08:45:52 UTC
OK, I'm happy with the logic here.  I had to go digging a little to prove to mysefl that the assumption was valid.  (Lines 530 -> 539 in commit http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt;hb=544520d6bdfacada4eba4caefedf42864db4b3a0#l529)

So, this patch does indeed fix the problem.. however as a side note, I really don't like seeing this kind of logic at the template level.. it's not testable and leads to repetative code :'(.

I'de rather see the template just ask the question 'to display, or not to display?', and have the actual logic involved in that question handled further up in the code, perferably in either C4/Suggestions.pm or as part of the Koha.preference plugin somehow.  That way, we'd have a testable fix.  Just a thought
Comment 9 Thomas Wright 2015-01-14 20:11:45 UTC
(In reply to Martin Renvoize from comment #8)
> OK, I'm happy with the logic here.  I had to go digging a little to prove to
> mysefl that the assumption was valid.  (Lines 530 -> 539 in commit
> http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=koha-tmpl/opac-
> tmpl/bootstrap/en/modules/opac-results.tt;
> hb=544520d6bdfacada4eba4caefedf42864db4b3a0#l529)
> 
> So, this patch does indeed fix the problem.. however as a side note, I
> really don't like seeing this kind of logic at the template level.. it's not
> testable and leads to repetative code :'(.
> 
> I'de rather see the template just ask the question 'to display, or not to
> display?', and have the actual logic involved in that question handled
> further up in the code, perferably in either C4/Suggestions.pm or as part of
> the Koha.preference plugin somehow.  That way, we'd have a testable fix. 
> Just a thought

Should I work on this?
Comment 10 Katrin Fischer 2015-01-14 20:13:05 UTC
I think not a blocker for your patch - just some thoughts about how we could handle things better.
Comment 11 Katrin Fischer 2015-01-15 06:14:17 UTC
Created attachment 35281 [details] [review]
[PASSED QA] Bug 12702 - Suggestions still accessible in OPAC when deactivated

Added check for suggestions turned off. If off user is redirected to 404 error. If patron suggestions are off guest sugestions will also redirect to 404

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Martin Renvoize 2015-01-15 09:58:35 UTC
Agreed, not a blocker here.. it's not your job to go cleaning the code up.. this was more just a comment trying to get anyone that sees it to think a bit more when implementing such code in the first place.. had such a request been centralised in the first place this fix probably wouldn't have been needed ;)

Cheers Thomas, you guys are doing a great job this year :)
Comment 13 Tomás Cohen Arazi 2015-01-18 13:55:18 UTC
Patch pushed to master.

Thanks Thomas!
Comment 14 Chris Cormack 2015-01-19 19:06:39 UTC
Pushed to 3.18.x will be in 3.18.3