Description
Jonathan Druart
2019-05-03 01:04:13 UTC
Created attachment 89264 [details] [review] Revert "Bug 22478: (QA follow-up) Update tests to check for any script tags" This reverts commit d9b3b90849fd95d296b73b330c4e32ff018c81a5. Created attachment 89265 [details] [review] Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean" This reverts commit 375dd35d5354d46c4c16f534494b123ce370c3f4. Created attachment 89266 [details] [review] Bug 22836: (follow-up) Bug 22478: (QA follow-up) Make test consistent with variable name One occurrence was missing. (In reply to Jonathan Druart from comment #2) > Created attachment 89265 [details] [review] [review] > Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a > boolean" > > This reverts commit 375dd35d5354d46c4c16f534494b123ce370c3f4. I don't see the sense of removing these lines. Hello Marcel, I have tried to explain it in the comment of this bug report. Which part does not make sense to you? (In reply to Jonathan Druart from comment #5) > Hello Marcel, > I have tried to explain it in the comment of this bug report. Which part > does not make sense to you? Improve filtering is great. But we do not need to remove solid code. (In reply to Marcel de Rooy from comment #6) > (In reply to Jonathan Druart from comment #5) > > Hello Marcel, > > I have tried to explain it in the comment of this bug report. Which part > > does not make sense to you? > > Improve filtering is great. But we do not need to remove solid code. Then we need to write more tests. I *strongly* think that we must remove this code. IMO It's not solid code, it's code which leads us to believe that the values are not correctly filtered. I've come to the conclusion that Jonathan is correct here. 1) We need to have a generic test for missing filters in pagination 2) The 'noise' created by not cleaning up the lines directly is inconsequential (in so much as it'll only ever appear if someone is attempting to XSS us AND the end user will never see it unless they're inspecting the html). Signing off. Created attachment 89677 [details] [review] Revert "Bug 22478: (QA follow-up) Update tests to check for any script tags" This reverts commit d9b3b90849fd95d296b73b330c4e32ff018c81a5. https://bugs.koha-community.org/show_bug.cgi?id=22836 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89678 [details] [review] Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean" This reverts commit 375dd35d5354d46c4c16f534494b123ce370c3f4. https://bugs.koha-community.org/show_bug.cgi?id=22836 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89679 [details] [review] Bug 22836: (follow-up) Bug 22478: (QA follow-up) Make test consistent with variable name One occurrence was missing. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89710 [details] [review] Revert "Bug 22478: (QA follow-up) Update tests to check for any script tags" This reverts commit d9b3b90849fd95d296b73b330c4e32ff018c81a5. https://bugs.koha-community.org/show_bug.cgi?id=22836 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 89711 [details] [review] Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean" This reverts commit 375dd35d5354d46c4c16f534494b123ce370c3f4. https://bugs.koha-community.org/show_bug.cgi?id=22836 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 89712 [details] [review] Bug 22836: (follow-up) Bug 22478: (QA follow-up) Make test consistent with variable name One occurrence was missing. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I am not sure about the commit messages - the first 2 are not referring to this bug. Leaving that to Nick ;) (In reply to Katrin Fischer from comment #16) > I am not sure about the commit messages - the first 2 are not referring to > this bug. Leaving that to Nick ;) They are ok, as they are reverting commits I think we should keep this syntax. Awesome work all! Pushed to master for 19.05 Pushed to 18.11.x for 18.11.06 |