Description
Nick Clemens (kidclamp)
2021-03-12 14:50:35 UTC
Created attachment 121439 [details] [review] Bug 27948: Add atomic update Created attachment 121440 [details] [review] Bug 27948: Add ArticleRequestsDisclaimerText syspref Created attachment 121441 [details] [review] Bug 27948: Add disclaimer to article request If ArticleRequestsDisclaimerText syspref is set, a disclaimer text will appear fefore an article request can be made To test: 1. apply patches 2. enable ArticleRequest syspref 3. in OPAC place an article request for a patron CHECK => No disclaimer text should appear before placing article request 4. add a text in ArticleRequestsDisclaimerText syspref 5. repeat step 3 SUCCESS => A disclaimer text is shown before you can place an article request Created attachment 121505 [details] [review] Bug 27948: Add atomic update Signed-off-by: David Nind <david@davidnind.com> Created attachment 121506 [details] [review] Bug 27948: Add ArticleRequestsDisclaimerText syspref Signed-off-by: David Nind <david@davidnind.com> Created attachment 121507 [details] [review] Bug 27948: Add disclaimer to article request If ArticleRequestsDisclaimerText syspref is set, a disclaimer text will appear fefore an article request can be made To test: 1. apply patches 2. enable ArticleRequest syspref 3. in OPAC place an article request for a patron CHECK => No disclaimer text should appear before placing article request 4. add a text in ArticleRequestsDisclaimerText syspref 5. repeat step 3 SUCCESS => A disclaimer text is shown before you can place an article request Signed-off-by: David Nind <david@davidnind.com> Hi Agustin, we have started moving the HTML system preferences into the news system as this allows to have translated versions of the text. i know this was modeled after the ILLModuleCopyrightClearance that hasn't been moved yet, but would you consider following the new pattern here? opacheader would be an example. Created attachment 122053 [details] [review] Bug 27948: Add disclaimer to article request If ArticleRequestsDisclaimerText news is set, a disclaimer text will appear before an article request can be made To test: 1. apply patches 2. enable ArticleRequest syspref 3. in OPAC place an article request for a patron CHECK => No disclaimer text should appear before placing article request 4. add a text in ArticleRequestsDisclaimerText news 5. repeat step 3 SUCCESS => A disclaimer text is shown before you can place an article request Signed-off-by: David Nind <david@davidnind.com> Could not compile /usr/share/koha/opac/opac-request-article.pl: Bareword "action" not allowed while "strict subs" in use at /usr/share/koha/opac/opac-request-article.pl line 49. How did you test that, Agustin? :) Why not use a construction like: [% PROCESS koha_news_block news => KohaNews.get({ location => 'ArticleRequestsDisclaimerText', lang => lang, library => branchcode, blocktitle => 0 }) %] This is used in other places too to insert text block from news. Created attachment 122132 [details] [review] Bug 27948: Add disclaimer to article request If ArticleRequestsDisclaimerText news is set, a disclaimer text will appear before an article request can be made To test: 1. apply patches 2. enable ArticleRequest syspref 3. in OPAC place an article request for a patron CHECK => No disclaimer text should appear before placing article request 4. add a text in ArticleRequestsDisclaimerText news 5. repeat step 3 SUCCESS => A disclaimer text is shown before you can place an article request Signed-off-by: David Nind <david@davidnind.com> Created attachment 122134 [details] [review] Bug 27948: Add disclaimer to article request If ArticleRequestsDisclaimerText news is set, a disclaimer text will appear before an article request can be made To test: 1. apply patches 2. enable ArticleRequest syspref 3. in OPAC place an article request for a patron CHECK => No disclaimer text should appear before placing article request 4. add a text in ArticleRequestsDisclaimerText news 5. repeat step 3 SUCCESS => A disclaimer text is shown before you can place an article request Signed-off-by: David Nind <david@davidnind.com> (In reply to Marcel de Rooy from comment #9) > Could not compile /usr/share/koha/opac/opac-request-article.pl: Bareword > "action" not allowed while "strict subs" in use at > /usr/share/koha/opac/opac-request-article.pl line 49. > > How did you test that, Agustin? :) Sorry, moved the if from the end of line to a proper if statement and forgot $ (In reply to Marcel de Rooy from comment #10) > Why not use a construction like: > > [% PROCESS koha_news_block news => KohaNews.get({ location => > 'ArticleRequestsDisclaimerText', lang => lang, library => branchcode, > blocktitle => 0 }) %] > > This is used in other places too to insert text block from news. Basically that construction does not allow for you to have a default language. If you add a disclaimer in english, but you do not have it in other languages, users with english lang will see the disclaimer text, but other users will not. With the construction I coded, it first checks for a disclaimer text in the language of the template, and if not found, uses english as default. Hi Augustin, I think we should not hardcode a fallback to English and it would be not correct with the other behaviour in Koha, like the templating system. Actually I think the fallback is usually to the first language in the list I think which is why you can reorder them? Maybe adding a fallback/default should be a separate bug - In general I agree it would be nice to have one in general. Bug 24766 might be related. Please note the fallback in NewsChannels too: (opac_news.lang = '' OR opac_news.lang = ?) (In reply to Marcel de Rooy from comment #16) > Please note the fallback in NewsChannels too: > > (opac_news.lang = '' OR opac_news.lang = ?) But it is not very useful here ;) I agree with Katrin that a fallback in cases like this would be nice, but it is not the scope of this report. Given how Koha now operates, I think we could well do without the fallback for now. If you did not define the disclaimer in a language that you do offer, you dont have a disclaimer. As soon as a solution for GetNewsToDisplay is found, it will resolve this case too..
> Given how Koha now operates, I think we could well do without the fallback
> for now. If you did not define the disclaimer in a language that you do
> offer, you dont have a disclaimer.
> As soon as a solution for GetNewsToDisplay is found, it will resolve this
> case too..
+1
(In reply to Katrin Fischer from comment #18) > > Given how Koha now operates, I think we could well do without the fallback > > for now. If you did not define the disclaimer in a language that you do > > offer, you dont have a disclaimer. > > As soon as a solution for GetNewsToDisplay is found, it will resolve this > > case too.. > > +1 Ok, seems I overdid here.. I'll try to keep it simple then. Created attachment 122519 [details] [review] Bug 27948: Add disclaimer to article request If ArticleRequestsDisclaimerText news is set, a disclaimer text will appear before an article request can be made To test: 1. apply patches 2. enable ArticleRequest syspref 3. in OPAC place an article request for a patron CHECK => No disclaimer text should appear before placing article request 4. add a text in ArticleRequestsDisclaimerText news 5. repeat step 3 SUCCESS => A disclaimer text is shown before you can place an article request Signed-off-by: David Nind <david@davidnind.com> Created attachment 122520 [details] [review] Bug 27948: (follow-up) Use Koha.news and PROCESS koha_news_block to bring disclaimer Created attachment 122523 [details] [review] Bug 27948: Add disclaimer to article request If ArticleRequestsDisclaimerText news is set, a disclaimer text will appear before an article request can be made To test: 1. apply patches 2. enable ArticleRequest syspref 3. in OPAC place an article request for a patron CHECK => No disclaimer text should appear before placing article request 4. add a text in ArticleRequestsDisclaimerText news 5. repeat step 3 SUCCESS => A disclaimer text is shown before you can place an article request Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 122524 [details] [review] Bug 27948: (follow-up) Use Koha.news and PROCESS koha_news_block to bring disclaimer Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 122525 [details] [review] Bug 27948: (QA follow-up) Remove unused $disclaimer code Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Actually, seeing a minor bug: +[% SET disclaimer = KohaNews.get( location => "ArticleRequestsDisclaimerText", lang => lang, library => branchcode ) %] Just as I did, you expected branchcode here already filled just like lang is. But unfortunately branchcode is empty. Which results in getting the All branches disclaimer only. I dont consider this as a blocker here, and I suspect that a few other calls of KohaNews.get suffer from the same assumption. So we should probably fill branchcode in Auth.pm like lang in Templates. Opening a new report for this. Created attachment 122526 [details] [review] Bug 27948: (QA follow-up) Correct empty branchcode Let us just fix it here and leave the rest to bug 28650. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Marcel de Rooy from comment #25) > I dont consider this as a blocker here, and I suspect that a few other calls > of KohaNews.get suffer from the same assumption. So we should probably fill > branchcode in Auth.pm like lang in Templates. > Opening a new report for this. See bug 28650 Agustin: When you have an All branches and a branch specific one, they are both listed directly following each other. An OPAC user would not know that they actually are two separate messages. Staff should be aware of that in order to prevent misunderstandings. No blocker. (In reply to Marcel de Rooy from comment #28) > Agustin: When you have an All branches and a branch specific one, they are > both listed directly following each other. An OPAC user would not know that > they actually are two separate messages. Staff should be aware of that in > order to prevent misunderstandings. > No blocker. I see.. maybe we could make a "KohaNews.getOne" helper with the same paremeters, but get the most relevant news.. if you get one that matches your library it fetches that, else the one for all branches. What do you think? (In reply to Agustín Moyano from comment #29) > I see.. maybe we could make a "KohaNews.getOne" helper with the same > paremeters, but get the most relevant news.. if you get one that matches > your library it fetches that, else the one for all branches. > > What do you think? Think we should let this be as-is. And perhaps discuss on another report if KohaNews.get should really fetch both in the first place instead of falling back to All branches if there is NO branch specific one.. Sorry but this does no longer apply. Bug 22544 has been pushed right after this bug has been PQAed and it removed C4::NewsChannels. You shouldn't need the 2 GetNewsToDisplay calls, Koha::News->search_for_display is supposed to deal with that. Created attachment 122771 [details] [review] Bug 27948: (QA follow-up) Remove NewsChannels reference We only need to remove the obsolete module here. Note that the two NewsToDisplay calls were removed already. Replaced by a KohaNews.get call. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 122774 [details] [review] Bug 27948: Remove unused branchcode var Pushed to master for 21.11, thanks to everybody involved! opac/opac-request-article.pl changed file mode from 100755 to 100644 Hum?? Created attachment 122795 [details] [review] Bug 27948: Fix file mode From commit b7acefa116c9ae1e738b5c53f30cf0d6c11d8784 Bug 27948: (follow-up) Use Koha.news and PROCESS koha_news_block to bring disclaimer opac/opac-request-article.pl changed file mode from 100755 to 100644 Follow-up pushed to master. Enhancement not backported for 21.05 |