Bug 27948 - Add disclaimer text to article requests feature
Summary: Add disclaimer text to article requests feature
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Agustín Moyano
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-03-12 14:50 UTC by Nick Clemens
Modified: 2021-07-16 12:34 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Text to go in the release notes:
This enhancement lets you include text that patrons need to accept before they can place an article request (similar to the ILLModuleCopyrightClearance system preference). Add the text required to the new ArticleRequestsDisclaimerText entry in the news tool.
Version(s) released in:
21.11.00


Attachments
Bug 27948: Add atomic update (1.33 KB, patch)
2021-05-26 19:06 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27948: Add ArticleRequestsDisclaimerText syspref (2.45 KB, patch)
2021-05-26 19:06 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27948: Add disclaimer to article request (23.76 KB, patch)
2021-05-26 19:06 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27948: Add atomic update (1.37 KB, patch)
2021-05-29 02:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 27948: Add ArticleRequestsDisclaimerText syspref (2.52 KB, patch)
2021-05-29 02:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 27948: Add disclaimer to article request (23.82 KB, patch)
2021-05-29 02:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 27948: Add disclaimer to article request (25.31 KB, patch)
2021-06-16 21:33 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27948: Add disclaimer to article request (25.32 KB, patch)
2021-06-18 13:11 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27948: Add disclaimer to article request (25.27 KB, patch)
2021-06-18 13:15 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27948: Add disclaimer to article request (25.35 KB, patch)
2021-07-02 00:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27948: (follow-up) Use Koha.news and PROCESS koha_news_block to bring disclaimer (3.40 KB, patch)
2021-07-02 00:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27948: Add disclaimer to article request (25.45 KB, patch)
2021-07-02 06:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27948: (follow-up) Use Koha.news and PROCESS koha_news_block to bring disclaimer (3.50 KB, patch)
2021-07-02 06:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27948: (QA follow-up) Remove unused $disclaimer code (1.17 KB, patch)
2021-07-02 06:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27948: (QA follow-up) Correct empty branchcode (1.24 KB, patch)
2021-07-02 07:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27948: (QA follow-up) Remove NewsChannels reference (887 bytes, patch)
2021-07-12 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27948: Remove unused branchcode var (950 bytes, patch)
2021-07-12 11:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27948: Fix file mode (690 bytes, patch)
2021-07-13 07:50 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-03-12 14:50:35 UTC
Similar to  ILLModuleCopyrightClearance  this feature will allow for including text about article request availability
Comment 1 Agustín Moyano 2021-05-26 19:06:33 UTC
Created attachment 121439 [details] [review]
Bug 27948: Add atomic update
Comment 2 Agustín Moyano 2021-05-26 19:06:38 UTC
Created attachment 121440 [details] [review]
Bug 27948: Add ArticleRequestsDisclaimerText syspref
Comment 3 Agustín Moyano 2021-05-26 19:06:43 UTC
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
Comment 4 David Nind 2021-05-29 02:32:39 UTC
Created attachment 121505 [details] [review]
Bug 27948: Add atomic update

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2021-05-29 02:32:43 UTC
Created attachment 121506 [details] [review]
Bug 27948: Add ArticleRequestsDisclaimerText syspref

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2021-05-29 02:32:47 UTC
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>
Comment 7 Katrin Fischer 2021-06-03 12:09:57 UTC
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.
Comment 8 Agustín Moyano 2021-06-16 21:33:28 UTC
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>
Comment 9 Marcel de Rooy 2021-06-17 06:22:13 UTC
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? :)
Comment 10 Marcel de Rooy 2021-06-17 06:29:31 UTC
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.
Comment 11 Agustín Moyano 2021-06-18 13:11:47 UTC
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>
Comment 12 Agustín Moyano 2021-06-18 13:15:40 UTC
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>
Comment 13 Agustín Moyano 2021-06-18 13:19:03 UTC
(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 $
Comment 14 Agustín Moyano 2021-06-18 13:39:52 UTC
(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.
Comment 15 Katrin Fischer 2021-06-19 10:48:32 UTC
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.
Comment 16 Marcel de Rooy 2021-06-21 09:10:31 UTC
Please note the fallback in NewsChannels too:

(opac_news.lang = '' OR opac_news.lang = ?)
Comment 17 Marcel de Rooy 2021-06-21 09:19:34 UTC
(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..
Comment 18 Katrin Fischer 2021-06-21 09:27:33 UTC
> 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
Comment 19 Agustín Moyano 2021-07-01 13:11:04 UTC
(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.
Comment 20 Agustín Moyano 2021-07-02 00:19:36 UTC
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>
Comment 21 Agustín Moyano 2021-07-02 00:19:40 UTC
Created attachment 122520 [details] [review]
Bug 27948: (follow-up) Use Koha.news and PROCESS koha_news_block to bring disclaimer
Comment 22 Marcel de Rooy 2021-07-02 06:22:26 UTC
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>
Comment 23 Marcel de Rooy 2021-07-02 06:22:30 UTC
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>
Comment 24 Marcel de Rooy 2021-07-02 06:22:34 UTC
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>
Comment 25 Marcel de Rooy 2021-07-02 06:52:42 UTC
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.
Comment 26 Marcel de Rooy 2021-07-02 07:07:25 UTC
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>
Comment 27 Marcel de Rooy 2021-07-02 07:08:02 UTC
(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
Comment 28 Marcel de Rooy 2021-07-02 07:14:34 UTC
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.
Comment 29 Agustín Moyano 2021-07-02 18:21:09 UTC
(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?
Comment 30 Marcel de Rooy 2021-07-05 05:58:26 UTC
(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..
Comment 31 Jonathan Druart 2021-07-09 13:05:48 UTC
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.
Comment 32 Marcel de Rooy 2021-07-12 07:59:01 UTC
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>
Comment 33 Jonathan Druart 2021-07-12 11:34:38 UTC
Created attachment 122774 [details] [review]
Bug 27948: Remove unused branchcode var
Comment 34 Jonathan Druart 2021-07-12 13:49:16 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 35 Jonathan Druart 2021-07-13 07:48:15 UTC
opac/opac-request-article.pl changed file mode from 100755 to 100644

Hum??
Comment 36 Jonathan Druart 2021-07-13 07:50:04 UTC
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
Comment 37 Jonathan Druart 2021-07-13 07:52:12 UTC
Follow-up pushed to master.
Comment 38 Kyle M Hall 2021-07-16 12:34:02 UTC
Enhancement not backported for 21.05