*** Bug 4881 has been marked as a duplicate of this bug. ***

I think this looks good, but there are some issues that need to be looked at:

I think that if there is no email address for the logged-in user's library the page should not offer to email the librarian. In my test if I submit a report to the librarian and there is no email address for my home library, the entry saved in message_queue has an empty 'to_address' field.

There are HTML validation errors triggered by duplicate id attributes on form fields. The form fields on the two different tabs should not have the same ids (note that input name and id attributes do not have to match);

Also, in the OPAC (and soon to be the case in the staff client), page-specific JavaScript goes into a special BLOCK which is processed in the footer include. So instead of this, at the end of opac-reportaproblem.tt:

[% BLOCK jsinclude %][% END %]

<script type="text/javascript">
    $(document).ready(function(){
        $("#reportproblem").tabs();
    });
</script>

It should be:

[% BLOCK jsinclude %]
    <script type="text/javascript">
        $(document).ready(function(){
            $("#reportproblem").tabs();
        });
    </script>
[% END %]

Patch doesn't apply :

Some problems occurred applying patches from bug 4461:
<h1>Something went wrong !</h1>Applying: Bug 4461: Add a context-sensitive 'Report a Problem' feature to OPAC
Using index info to reconstruct a base tree...
M       Koha/Schema/Result/Borrower.pm
M       installer/data/mysql/kohastructure.sql
M       koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging Koha/Schema/Result/Borrower.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/Borrower.pm
Patch failed at 0001 Bug 4461: Add a context-sensitive 'Report a Problem' feature to OPAC
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
error: Failed to merge in the changes.
Bug 4461 - Context-sensitive report a problem screen

59032 - Bug 4461: Add a context-sensitive 'Report a Problem' feature to OPAC

Hi Aleisha,

this doesn't apply. It is recommended to have the changes to the Schema in a separate patch, because it's likely to conflict a lot. This way one can opt to not apply the patch and run the schema update locally when testing or pushing and you don't have to rebase as often.
https://wiki.koha-community.org/wiki/Database_updates#Changes_with_DBIx::Class

Ok... partially talking nonsense. It DOES currently apply!

Really like the feature! Maybe we should add something like this to staff too?

Possible problems:
- Instead of tabs have a simple pull down to switch the recipient. At the moment, if you have typed up your message and then change your mind about whom to send it to, you have to start over as the text won't carry over from one tab to the other.
- When submitting a problem report to the aministrator and there is no branch email supplied, the 'success' message and the 'error' message will both show at the same time after submitting. This might be confusing to the patron. Maybe we could avoid that by showing the 'error' next to the submit button or if we didn't have the tabs, not show the option to email the library at all, when there is no email address set for the branch?
- There are some constraints with email sending if you are hosted and using the provider's mail server. So a while ago we have added branchreplyto (branches) and ReplyToDefault (system preference). It would be cool if you could check those first and email there if they are set and then fall back to branchemal and KohaAdminEmailAddress (like it's done for overdues etc.)
- If you are not logged in when clicking on 'report a bug' the problem will be reported against the opac-reportproblem page.

Questions:
- I know that the feature can be hidden using the id (thx!), but maybe this should also be a preference? 
- How will the table in the database be maintained? Is there a way to empty it or plans to make it accessible from the GUI?

Forgot to change status - this is ready to test

Created attachment 66323 [details] [review]
[SIGNED-OFF] Bug 4461: Add a context-sensitive 'Report a Problem' feature to OPAC

This patch adds a 'Report a problem' link to the bottom of every page in
the OPAC. The user can choose to submit their report to the library or
Koha administrator. The report will store the user's problem message,
but also their username and the page they were on when they clicked the
'report a problem' link. The patch adds a table 'problem_reports' to the
db.

Update: This patch adds a check for the KohaAdminEmailAddress and
library branch email before sending the report.
Update: Fix merge conflicts

To test:
1) Apply patch, update database, rebuild schema
2) Restart koha-common and memcached
3) Confirm that your user has an email address.
4) Confirm that your library does NOT have an email address.
5) Confirm that your syspref KohaAdminEmailAddress is NOT filled.
6) Log into OPAC
7) Click the 'Report a problem' link at the bottom of whatever page
you're on
8) Notice that there are error messages alerting that library does not
have an email address and that the Koha Administrator does not have
email address. Both submit buttons will be disabled.
9) Add an email address for your library. You may have to restart
memcached again. The error message for the library email address will
have disappeared and the Submit button for 'Report to library' will now
work.
10) Add an email address for the syspref KohaAdminEmailAddress. You may
have to restart memcached again. The error message for the administrator
email address will have disappeared and the Submit button for 'Report to
Koha Administrator' will not work.
11) First try submitting a problem to the library. Confirm that the
fields for 'Problem found on page' and 'Username' are correct, then fill
out the 'Subject' and 'Message' and click Submit.
12) Confirm a message shows at the top of the page to show success.
13) Go into mysql and display records from the message_queue table.
Confirm that the borrowernumber and subject are correct. Confirm that
content shows the message from the problem report, but also the user's
username and the problem page. Confirm that the to_address shows the
library's email address and the from_address shows the user's email
address.
14) Now try submitting a problem to the Koha Admin. Fill fields and
click Submit. Confirm message shows at top to show success again.
15) Check message_queue for new problem report. Confirm that the
to_address shows the email address from the syspref
KohaAdminEmailAddress.

Sponsored-by: Catalyst IT

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 66324 [details] [review]
[SIGNED-OFF] Bug 4461: [FOLLOW-UP] Problem Report feature

This patch:
 - uses a pull down menu to switch the recipient instead of tabs
 - if the library has no email or the administrator has no email, this
 error message has been moved to be down by the Submit button
 - includes use of branchreplyto and ReplytoDefault for branch email
 - disables use of the feature if the user is not logged in
 - adds a syspref to hide or show the feature
 - provides support for if JS is disabled

Additional testing:
 - using the message_queue table, check that the correct recipient
 receives a problem report when switching in the drop down menu
 - check appropriate error message shows next to the submit button if an
 email is not provided. this should dynamically change as you select a
 different recipient from the drop down menu
 - check that when the library is the recipient, the feature sends the
 report to the branchreplyto, then the ReplytoDefault, then the branch
 email in that priority order
 - log out of the OPAC and confirm that you can't submit a report
 without logging in. the submit button should be disabled
 - disable the OPACReportProblem syspref. the Report a Problem link in
 the footer should be gone. if you force the URL
 (/cgi-bin/koha/opac-reportproblem.pl) the form should be hidden and
 replaced with an error message.
 - disable JS in your browser. confirm that you are still able to send
 problem reports to library and administrator. remove the administrator
 email address. confirm the error message shows next to the submit
 button and that clicking the submit button returns an error. check in
 the message_queue that the email was never sent.

After this feature is passed:
 - i would like to add a staff-side feature where staff can see the
 problem reports and mark them as 'read'. i'll work on that once this
 gets through :)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

(In reply to Aleisha Amohia from comment #11)

> After this feature is passed:
>  - i would like to add a staff-side feature where staff can see the
>  problem reports and mark them as 'read'. i'll work on that once this
>  gets through :)

Great idea, I am really looking forward to ;)

This is a *bit* off topic, but I've been working on a plugin that fills a similar niche on the staff side: https://github.com/bywatersolutions/koha-plugin-support

Hi Aleisha,

1 - Need tests for new Objects

2 - With 'DefaultReplyTo' unset it is still 'defined' so I get the library email option even when none are set.

3 - If not logged in I still see the report form, I think I should just get the warning to log in.

3 - Similarly, if both emails are undefined maybe I shouldn't get the form at all since I can't submit?

4 - Rather than a JS function to disable the submit button can we just disable the input on the dropdown and add a tooltip? Or only provide a dropdown if there is a choice possible? The warning about missing emails seems more appropriate for the about page than the users.

5 - The new preference is defaulting to 'Allow' it should default to 'Don't Allow'

Created attachment 98687 [details] [review]
Bug 4461: Database and syspref changes

adding problem_reports table and OPACReportProblem syspref

Created attachment 98688 [details] [review]
Bug 4461: Problem Report Koha objects

Created attachment 98689 [details] [review]
Bug 4461: Submit a problem report

To test:
1) Apply patch, update database, rebuild schema file
2) Restart koha-common and memcached
3) Confirm that your user has an email address.
4) Confirm that your library does NOT have an email address.
5) Confirm that syspref KohaAdminEmailAddress and syspref ReplytoDefault are not filled. Enable the OPACReportProblem syspref.
6) Log into OPAC
7) Click the 'Report a problem' link at the bottom of whatever page
you're on
8) Notice that there is no form is and there is an error message alerting that reports cannot be submitted
9) Add an email address for your library (in either the email field or the replyto field). Refresh the OPAC problem report page. The form should now show, and the recipient field should say 'library'.
10) Complete the form and submit. Check the message_queue in the database and confirm the to_address is correct. Confirm there is a success message.
11) Add an email address for the syspref KohaAdminEmailAddress and refresh the OPAC problem report page again. The recipient field should now be a dropdown. Select the Koha Administrator option.
12) Complete the form and submit. Check the message_queue in the database and confirm the to_address is the value in KohaAdminEmailAddress. Confirm there is a success message.
13) Ensure all details in the message_queue are correct.
14) Log out of the OPAC
15) Click the Report a problem link again and confirm you are forced to log in

Sponsored-by: Catalyst IT

This looks like it's working well. I have only one issue: Based on how we handle it with other features/pages which can be deactivated I think this page should return a 404 error if it's not active.

Created attachment 98725 [details] [review]
Bug 4461: (follow-up) Redirect to 404 if OPACProblemReport not enabled

The sandbox got mad about letting me sign off, but I followed the test plan successfully. Signing off!

Hi Aleisha, 

starting with review and QA tools here:

1) Can you check if this is a real one or a false positive? 
 FAIL	Koha/ProblemReport.pm
   FAIL	  pod coverage
		POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666


2) Please fix filters:

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reportproblem.tt
   FAIL	  filters
		missing_filter at line 3 (<title>[% IF ( LibraryNameTitle ) %][% LibraryNameTitle %][% ELSE %]Koha online[% END %] catalog &rsaquo; Report a Problem</title>)
		missing_filter at line 31 (                        <div class="alert alert-info">Your problem report has been sent to the [% IF ( recipient == 'admin' ) %]Koha Administrator[% ELSE %][% recipient %][% END %].</div>)
		missing_filter at line 56 (                                            <input type="hidden" name="place" id="place" value="[% probpage %]">)
		missing_filter at line 61 (                                            <input type="hidden" name="user" id="user" value="[% username %]" class="span3">)
		missing_filter at line 65 (                                            <input type="text" name="subject" id="subject" value="[% subject %]" class="span3">)


3) Database update

You got:
+    $dbh->do(q{ DROP TABLE IF EXISTS `problem_reports` });

I think this could be destructive in some cases. I think checking for existence of the table is better:

20126     if ( !TableExists('return_claims') ) {


4) Capitalization

catalog &rsaquo; Report a Problem</title>
Koha Administrator

5)   print $input->redirect("/cgi-bin/koha/opac-reportproblem?norecipients=1.pl");

Looks like a typo with the .pl at the end?

6) Translatatability

I these go in the email to the library/admin, but won't be translatable: 

+    $problemreport->{code} = 'PROBLEM_REPORT';
+    $problemreport->{content} .= "\nUsername: $username";
+    $problemreport->{content} .= "\nProblem page: $place";

Using a notice template would be the most elegant way to make those translatable. I think with https://bugs.koha-community.org/bugzilla3/showdependencytree.cgi?id=15395&hide_resolved=1 you can not yet add strings to .pl fils in a translatable way.


If I understand correctly the only way to access the reports for now would be a report? It might be nice (new bug!) to have this on the dashoard and visible in staff or even the patron account :)

Thanks Katrin I'll work on fixing those. The staff side follow-up was already in my plan:

(In reply to Aleisha Amohia from comment #11)

> After this feature is passed:
>  - i would like to add a staff-side feature where staff can see the
>  problem reports and mark them as 'read'. i'll work on that once this
>  gets through :)

Created attachment 99551 [details] [review]
Bug 4461: (follow-up) Creating sample notice and other fixes

- filters
- capitalisation
- atomic update

(In reply to Katrin Fischer from comment #24)
> Hi Aleisha, 
> 
> starting with review and QA tools here:
> 
> 1) Can you check if this is a real one or a false positive? 
>  FAIL	Koha/ProblemReport.pm
>    FAIL	  pod coverage
> 		POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
> 

i dont really know how i could expand on the documentation i've already done!

(In reply to Aleisha Amohia from comment #27)
> (In reply to Katrin Fischer from comment #24)
> > Hi Aleisha, 
> > 
> > starting with review and QA tools here:
> > 
> > 1) Can you check if this is a real one or a false positive? 
> >  FAIL	Koha/ProblemReport.pm
> >    FAIL	  pod coverage
> > 		POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
> > 
> 
> i dont really know how i could expand on the documentation i've already done!

That's ok, we get those as false positives sometimes.

Created attachment 99599 [details] [review]
Bug 4461: Database and syspref changes

adding problem_reports table and OPACReportProblem syspref

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 99600 [details] [review]
Bug 4461: Problem Report Koha objects

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 99601 [details] [review]
Bug 4461: Submit a problem report

To test:
1) Apply patch, update database, rebuild schema file
2) Restart koha-common and memcached
3) Confirm that your user has an email address.
4) Confirm that your library does NOT have an email address.
5) Confirm that syspref KohaAdminEmailAddress and syspref ReplytoDefault are not filled. Enable the OPACReportProblem syspref.
6) Log into OPAC
7) Click the 'Report a problem' link at the bottom of whatever page
you're on
8) Notice that there is no form is and there is an error message alerting that reports cannot be submitted
9) Add an email address for your library (in either the email field or the replyto field). Refresh the OPAC problem report page. The form should now show, and the recipient field should say 'library'.
10) Complete the form and submit. Check the message_queue in the database and confirm the to_address is correct. Confirm there is a success message.
11) Add an email address for the syspref KohaAdminEmailAddress and refresh the OPAC problem report page again. The recipient field should now be a dropdown. Select the Koha Administrator option.
12) Complete the form and submit. Check the message_queue in the database and confirm the to_address is the value in KohaAdminEmailAddress. Confirm there is a success message.
13) Ensure all details in the message_queue are correct.
14) Log out of the OPAC
15) Click the Report a problem link again and confirm you are forced to log in

Sponsored-by: Catalyst IT

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 99602 [details] [review]
Bug 4461: (follow-up) Redirect to 404 if OPACProblemReport not enabled

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 99603 [details] [review]
Bug 4461: (follow-up) Creating sample notice and other fixes

- filters
- capitalisation
- atomic update

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 99633 [details] [review]
Bug 4461: Rename report date with created_on

And make it a timestamp type column

Some notes:

1. reportdate must be created_on, with a type "timestamp".

2. opac/opac-reportproblem.pl

112     my $from_address = $member->email || $member->emailpro || $member->B_email || $koha_admin;

We usually do not specify the from_address on enqueuing the notice, but when sending it (from _send_message_by_email). That way a) the code is centralized (as we must use $patron->notice_email_address) and b) the email used is the one that is in the DB when the letter is effectively sent.
Here I guess you need to give it when the notice is enqueued as you want to default to the koha admin address.

I do not think it's a good idea, we should not send an email with "from" if not really sent by the user.
We have the borrowernumber anyway to know who filled the form.

I am trying to submit follow-ups.

Created attachment 99635 [details] [review]
Bug 4461: Use KohaAdminEmailAddress pref as 'from'

We usually do not specify the from_address on enqueuing the notice, but
when sending it (from _send_message_by_email). That way a) the code is
centralized (as we must use $patron->notice_email_address) and b) the
email used is the one that is in the DB when the letter is effectively
sent.
Here I guess you need to give it when the notice is enqueued as you want
to default to the koha admin address.

I do not think it's a good idea, we should not send an email with "from"
if not really sent by the user.
We have the borrowernumber anyway to know who filled the form.

Created attachment 99636 [details] [review]
Bug 4461: Use our local_referer instead of HTTP_REFERER

Cannot remember why exactly, but it seems that we should not use
HTTP_REFERER directly.

Created attachment 99637 [details] [review]
Bug 4461: Redirect to 404 if KohaAdminEmailAddress is not defined

Created attachment 99638 [details] [review]
Bug 4461: Simplify recipients code

As the feature is now turned off without KohaAdminEmailAddress, we only
need to check if the email address of the library if defined.
Not that we should not check for $library->branchreplyto or
ReplytoDefault, but only $library->branchemail, as we do everywhere else
(I think)

Created attachment 99639 [details] [review]
Bug 4461: Better error handling

I have attached few follow-ups, pick the ones you need/want.

However I am a bit at lost here.

There is no UI so far, but we have such things:
+        status varchar(1) NOT NULL default 'N', -- status of the report. N=new, V=viewed, C=closed

so the status will never be updated, what will happen when we will have the UI?

IMO we should bring the UI with that patchset, and remove the notice. Do we really want the notice to be sent? If yes, do we really want the new table? :)

Also, do we really need the problem_reports.recipient DB field if we have the email?
Same for username? We have the borrowernumber.

About the DB structure again, it seems that status should be an ENUM.

(In reply to Jonathan Druart from comment #42)
> I have attached few follow-ups, pick the ones you need/want.
> 
> However I am a bit at lost here.
> 
> There is no UI so far, but we have such things:
> +        status varchar(1) NOT NULL default 'N', -- status of the report.
> N=new, V=viewed, C=closed
> 
> so the status will never be updated, what will happen when we will have the
> UI?
> 
> IMO we should bring the UI with that patchset, and remove the notice. Do we
> really want the notice to be sent? If yes, do we really want the new table?
> :)

As mentioned in multiple earlier comments, once this is pushed I want to add a follow-up feature where staff can manage problem reports on the intranet. The original report for this feature asked for an email to be sent so that's what I implemented. Up to the community if they would prefer the email notice version or the staff-side management version, but I don't see any harm in having both.

> 
> Also, do we really need the problem_reports.recipient DB field if we have
> the email?
> Same for username? We have the borrowernumber.
> 
> About the DB structure again, it seems that status should be an ENUM.

Agreed that some of these fields are redundant, and status can be an ENUM. Happy to make these changes.

What do others think? Could we get your opinions?

(In reply to Jonathan Druart from comment #44)
> What do others think? Could we get your opinions?

I think having option to get email notices as well as having the staff interface is useful - I don't think we need to pick only one - but once there is a GUI the notices should be optional

>Bug 4461: Use KohaAdminEmailAddress pref as 'from'
I don't understand that followup - 'from_address' is just picked form the message in C4/Letters - it is 'to_address' that usually gets filled at send - to be to the borrowernumber

In this case I think we could set it as:
$patron->notice_email_address || $koha_admin;

(In reply to Nick Clemens from comment #45)
> (In reply to Jonathan Druart from comment #44)
> > What do others think? Could we get your opinions?
> 
> I think having option to get email notices as well as having the staff
> interface is useful - I don't think we need to pick only one - but once
> there is a GUI the notices should be optional

The problem with having the GUI after is that we do not deal with some of the already created fields.
See comment 42: "the status will never be updated, what will happen when we will have the UI?"

> >Bug 4461: Use KohaAdminEmailAddress pref as 'from'
> I don't understand that followup - 'from_address' is just picked form the
> message in C4/Letters - it is 'to_address' that usually gets filled at send
> - to be to the borrowernumber
> 
> In this case I think we could set it as:
> $patron->notice_email_address || $koha_admin;

I do not think so, as said in the commit message "I do not think it's a good idea, we should not send an email with "from" if not really sent by the user." I think it's bad practice to do that, I may be wrong however.

If Aleisha says the GUI will be next, I am not so worried about the status and such. It would be nice to see the code, but I feel this was just planning ahead and would not cause harm for now.

> > In this case I think we could set it as:
> > $patron->notice_email_address || $koha_admin;
> 
> I do not think so, as said in the commit message "I do not think it's a good
> idea, we should not send an email with "from" if not really sent by the
> user." I think it's bad practice to do that, I may be wrong however.

I agree here. To avoid being rejected as spam you can use the reporting patron's email as replyto, but not as From. So it's more than a bad practice as a lot of mail servers got a lot stricter in the last years - it will be real trouble (been there). From and Return-path needs to be from the mail servers domain. See also:
https://wiki.koha-community.org/wiki/MRenvoize/Email

I documented some of the ReplyTo stuff back when it was first introduced as a feature in bug 9530.. Jonathan is correct, we should not spoof user address in the from address is it's very likely you'll end up in a spam list that way.

https://wiki.koha-community.org/wiki/MRenvoize/Email roughly explains it and I have a feeling that was lifted and put in our own manual somewhere but I can't see it right now.

In bug 22821 I add some handling to allow specifying reply-to address at the EnqueuLetter stage.. it is my feeling if you want to send email 'from' a patron 'to' a library you should use a combination of not supplying a 'from' address (as it'll pick an appropriate one in _send_message_by_email), setting the 'to' using Koha::Library->get_effective_email (perhaps I should rename that get_inbound_email_address) which will do an appropriate fallback through replyto addresses and finally add a reply-to address of the patron so whoever receives the email can reply to the actual user.

See bug 22821 and bug 22823 for some further details around email handling.

(In reply to Katrin Fischer from comment #47)
> If Aleisha says the GUI will be next, I am not so worried about the status
> and such. It would be nice to see the code, but I feel this was just
> planning ahead and would not cause harm for now.

For now no. But when the GUI will be there the message will be marked as "new" even if they have been processed already. If we mark them all as "processed" when the GUI is added, that will also be wrong: some may have not still been processed.

Comment on attachment 99637 [details] [review]
Bug 4461: Redirect to 404 if KohaAdminEmailAddress is not defined

Review of attachment 99637 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
@@ +376,5 @@
>                    yes: Allow
>                    no: "Don't allow"
>              - patrons to submit problem reports for OPAC pages to the library or Koha Administrator.
> +            - "<br/>"
> +            - "<strong>NOTE:</strong> You must have KohaAdminEmailAddress enabled."

This isn't actually true is it?.. we use branch email in preference to KohaAdminEmailAddress so it's just a fallback not a hard requirement.

::: opac/opac-reportproblem.pl
@@ +41,5 @@
>  );
>  
> +if (   !C4::Context->preference('OPACReportProblem')
> +    || !C4::Context->preference('KohaAdminEmailAddress') )
> +{

Don't we actually use branchemail in preference to KohaAdminEmailAddress so this isn't actually a hard requirement?

Comment on attachment 99638 [details] [review]
Bug 4461: Simplify recipients code

Review of attachment 99638 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reportproblem.tt
@@ +32,5 @@
> +                        [% IF recipient == 'admin' %]
> +                            Your problem report has been sent to the Koha administrator.
> +                        [% ELSE %]
> +                            Your problem report has been sent to the library.
> +                        [% END %]

Do we really need to get this specific in the OPAC?

I'd say we just need to say whether the email was successfully sent or not and not worry so much whether it's KohaAdminEmail or Library Branch Email.. the end user doesn't have a way to select who it goes to anyway.

If we remove this we could simplify the code a little

(In reply to Jonathan Druart from comment #49)
> (In reply to Katrin Fischer from comment #47)
> > If Aleisha says the GUI will be next, I am not so worried about the status
> > and such. It would be nice to see the code, but I feel this was just
> > planning ahead and would not cause harm for now.
> 
> For now no. But when the GUI will be there the message will be marked as
> "new" even if they have been processed already. If we mark them all as
> "processed" when the GUI is added, that will also be wrong: some may have
> not still been processed.

Agreed, without any way of maintaining the status, I think we should remove it from this bug.. I like the idea of having a UI with Koha for progressing and tracking reported issues but until that time we shouldn't record incorrect status's.

(In reply to Martin Renvoize from comment #50)
> Comment on attachment 99637 [details] [review] [review]
> Bug 4461: Redirect to 404 if KohaAdminEmailAddress is not defined
> 
> Review of attachment 99637 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
> @@ +376,5 @@
> >                    yes: Allow
> >                    no: "Don't allow"
> >              - patrons to submit problem reports for OPAC pages to the library or Koha Administrator.
> > +            - "<br/>"
> > +            - "<strong>NOTE:</strong> You must have KohaAdminEmailAddress enabled."
> 
> This isn't actually true is it?.. we use branch email in preference to
> KohaAdminEmailAddress so it's just a fallback not a hard requirement.

With the 404 redirect in opac-reportproblem.pl it is a hard requirement.
Original code was assuming the pref was filled IIRC.

> ::: opac/opac-reportproblem.pl
> @@ +41,5 @@
> >  );
> >  
> > +if (   !C4::Context->preference('OPACReportProblem')
> > +    || !C4::Context->preference('KohaAdminEmailAddress') )
> > +{
> 
> Don't we actually use branchemail in preference to KohaAdminEmailAddress so
> this isn't actually a hard requirement?

Not what the code is actually doing. Maybe it needs to be adjusted.
105                         to_address             => C4::Context->preference('KohaAdminEmailAddress'),

(In reply to Martin Renvoize from comment #51)
> Comment on attachment 99638 [details] [review] [review]
> Bug 4461: Simplify recipients code
> 
> Review of attachment 99638 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reportproblem.tt
> @@ +32,5 @@
> > +                        [% IF recipient == 'admin' %]
> > +                            Your problem report has been sent to the Koha administrator.
> > +                        [% ELSE %]
> > +                            Your problem report has been sent to the library.
> > +                        [% END %]
> 
> Do we really need to get this specific in the OPAC?
> 
> I'd say we just need to say whether the email was successfully sent or not
> and not worry so much whether it's KohaAdminEmail or Library Branch Email..
> the end user doesn't have a way to select who it goes to anyway.
> 
> If we remove this we could simplify the code a little

Feel free to submit any follow-ups you wish ;)

Bug say's it's sponsored.. can we add a Sponsored-by line to at least one of the patches ;)

(In reply to Jonathan Druart from comment #49)
> (In reply to Katrin Fischer from comment #47)
> > If Aleisha says the GUI will be next, I am not so worried about the status
> > and such. It would be nice to see the code, but I feel this was just
> > planning ahead and would not cause harm for now.
> 
> For now no. But when the GUI will be there the message will be marked as
> "new" even if they have been processed already. If we mark them all as
> "processed" when the GUI is added, that will also be wrong: some may have
> not still been processed.

But that problem will be the same when we add the status column later on - if it's there now or not. I'd just aim for having the GUI same release if possible? I don't see the big issue here I'd rather spent the time to build the GUI then to make it work perfectly without now.

It also requires 'OpacKohaUrl' to be enabled.. that feels like an error.

Followups coming.. this has turned into a todo list for myself to help get this past the line.

Created attachment 99929 [details] [review]
Bug 4461: Database and syspref changes

adding problem_reports table and OPACReportProblem syspref

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

Created attachment 99930 [details] [review]
Bug 4461: Problem Report Koha objects

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

Created attachment 99931 [details] [review]
Bug 4461: Submit a problem report

To test:
1) Apply patch, update database, rebuild schema file
2) Restart koha-common and memcached
3) Confirm that your user has an email address.
4) Confirm that your library does NOT have an email address.
5) Confirm that syspref KohaAdminEmailAddress and syspref ReplytoDefault are not filled. Enable the OPACReportProblem syspref.
6) Log into OPAC
7) Click the 'Report a problem' link at the bottom of whatever page
you're on
8) Notice that there is no form is and there is an error message alerting that reports cannot be submitted
9) Add an email address for your library (in either the email field or the replyto field). Refresh the OPAC problem report page. The form should now show, and the recipient field should say 'library'.
10) Complete the form and submit. Check the message_queue in the database and confirm the to_address is correct. Confirm there is a success message.
11) Add an email address for the syspref KohaAdminEmailAddress and refresh the OPAC problem report page again. The recipient field should now be a dropdown. Select the Koha Administrator option.
12) Complete the form and submit. Check the message_queue in the database and confirm the to_address is the value in KohaAdminEmailAddress. Confirm there is a success message.
13) Ensure all details in the message_queue are correct.
14) Log out of the OPAC
15) Click the Report a problem link again and confirm you are forced to log in

Sponsored-by: Catalyst IT
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 99932 [details] [review]
Bug 4461: (follow-up) Redirect to 404 if OPACProblemReport not enabled

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

Created attachment 99933 [details] [review]
Bug 4461: (follow-up) Creating sample notice and other fixes

- filters
- capitalisation
- atomic update

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

Created attachment 99934 [details] [review]
Bug 4461: Rename report date with created_on

And make it a timestamp type column

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

Created attachment 99935 [details] [review]
Bug 4461: Use KohaAdminEmailAddress pref as 'from'

We usually do not specify the from_address on enqueuing the notice, but
when sending it (from _send_message_by_email). That way a) the code is
centralized (as we must use $patron->notice_email_address) and b) the
email used is the one that is in the DB when the letter is effectively
sent.
Here I guess you need to give it when the notice is enqueued as you want
to default to the koha admin address.

I do not think it's a good idea, we should not send an email with "from"
if not really sent by the user.
We have the borrowernumber anyway to know who filled the form.

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

Created attachment 99936 [details] [review]
Bug 4461: Use our local_referer instead of HTTP_REFERER

Cannot remember why exactly, but it seems that we should not use
HTTP_REFERER directly.

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

Created attachment 99937 [details] [review]
Bug 4461: Redirect to 404 if KohaAdminEmailAddress is not defined

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

Created attachment 99938 [details] [review]
Bug 4461: Simplify recipients code

As the feature is now turned off without KohaAdminEmailAddress, we only
need to check if the email address of the library if defined.
Not that we should not check for $library->branchreplyto or
ReplytoDefault, but only $library->branchemail, as we do everywhere else
(I think)

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

Created attachment 99939 [details] [review]
Bug 4461: Better error handling

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

Created attachment 99940 [details] [review]
Bug 4461: Remove hard requirement on OpacKohaUrl

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

Created attachment 99941 [details] [review]
Bug 4461: Drop 'from_address' use 'reply_address'

This patch adds support for using the reply-to field added in bug 10269
and drops 'from_address' as it will get defined at a later stage in the
emailing process anyway.

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

Created attachment 99942 [details] [review]
Bug 4461: Use get_effective_email from bug 22823

Use the get_effective_email from Koha::Library to get an appropriate
inbound address for the branch as aposed to using branchemail directly.

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

I'm reasonably happy with the followups here.. it feels like this was written with a GUI and a process in mind but without submitting the GUI half and as such the code is a little confusing.. why not just send emails and skip the table element entirely ;)

Still.. I like the feature and at this point, the code seems to work reasonably well.. I'd love to see a GUI including a process for moving reports through various states added this cycle if possible.

We keep getting stuck on the lack of GUI. I understand. I'll write the GUI for this now so it can all go through in one go.

Created attachment 100488 [details] [review]
Bug 4461: (follow-up) Creating sample notice and other fixes

- filters
- capitalisation
- atomic update

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

Created attachment 100489 [details] [review]
Bug 4461: Rename report date with created_on

And make it a timestamp type column

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

Created attachment 100490 [details] [review]
Bug 4461: Use KohaAdminEmailAddress pref as 'from'

We usually do not specify the from_address on enqueuing the notice, but
when sending it (from _send_message_by_email). That way a) the code is
centralized (as we must use $patron->notice_email_address) and b) the
email used is the one that is in the DB when the letter is effectively
sent.
Here I guess you need to give it when the notice is enqueued as you want
to default to the koha admin address.

I do not think it's a good idea, we should not send an email with "from"
if not really sent by the user.
We have the borrowernumber anyway to know who filled the form.

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

Created attachment 100491 [details] [review]
Bug 4461: Use our local_referer instead of HTTP_REFERER

Cannot remember why exactly, but it seems that we should not use
HTTP_REFERER directly.

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

Created attachment 100492 [details] [review]
Bug 4461: Redirect to 404 if KohaAdminEmailAddress is not defined

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

Created attachment 100493 [details] [review]
Bug 4461: Simplify recipients code

As the feature is now turned off without KohaAdminEmailAddress, we only
need to check if the email address of the library if defined.
Not that we should not check for $library->branchreplyto or
ReplytoDefault, but only $library->branchemail, as we do everywhere else
(I think)

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

Created attachment 100494 [details] [review]
Bug 4461: Better error handling

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

Created attachment 100495 [details] [review]
Bug 4461: Remove hard requirement on OpacKohaUrl

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

Created attachment 100496 [details] [review]
Bug 4461: Drop 'from_address' use 'reply_address'

This patch adds support for using the reply-to field added in bug 10269
and drops 'from_address' as it will get defined at a later stage in the
emailing process anyway.

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

Created attachment 100497 [details] [review]
Bug 4461: Use get_effective_email from bug 22823

Use the get_effective_email from Koha::Library to get an appropriate
inbound address for the branch as aposed to using branchemail directly.

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

Created attachment 100498 [details] [review]
Bug 4461: Adding user flags for managing OPAC problem reports

Created attachment 100499 [details] [review]
Bug 4461: Add problem reports link to mainpage and admin home

Created attachment 100500 [details] [review]
Bug 4461: Manage problem reports on the staff client

Test plan:

- Update database and upgrade schema files (if you haven't already).
Restart memcached
- Check your user's permissions and ensure the 'problem_reports'
permission is ticked. Confirm the OPACReportProblem syspref is enabled
- Log into the OPAC and submit a problem report
- Log into the staff client
- You should see a box at the bottom of the main page showing your
pending problem report
- Click the link and confirm it takes you to the new page for managing
problem reports
- Go to Administration
- Confirm you can see a link to 'OPAC problem reports' under the
'Additional parameters' heading
- Click 'OPAC problem reports'
- Confirm your problem report is showing in the table
- Open the OPAC in another tab and submit at least two more problem
reports (so you should have at least three in the table after
refreshing)
- Try the different buttons
    - selecting multiple problem reports and using the big 'mark
    viewed', 'mark closed', 'mark new' buttons. Confirm there are no
    failures and that the number of selected problem reports is correct
    - select all, clear all, hide viewed, hide closed, hide new, show
    all
    - individual 'mark viewed', 'mark closed', 'mark new' buttons for
    each problem report. Confirm the status shows and the correct button
    is disabled while others are enabled
- Confirm the problem page link works as expected

Sponsored-by: Catalyst IT

Created attachment 100506 [details] [review]
Bug 4461: Database and syspref changes

adding problem_reports table and OPACReportProblem syspref

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

Created attachment 100507 [details] [review]
Bug 4461: Problem Report Koha objects

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

Created attachment 100508 [details] [review]
Bug 4461: Submit a problem report

To test:
1) Apply patch, update database, rebuild schema file
2) Restart koha-common and memcached
3) Confirm that your user has an email address.
4) Confirm that your library does NOT have an email address.
5) Confirm that syspref KohaAdminEmailAddress and syspref ReplytoDefault are not filled. Enable the OPACReportProblem syspref.
6) Log into OPAC
7) Click the 'Report a problem' link at the bottom of whatever page
you're on
8) Notice that there is no form is and there is an error message alerting that reports cannot be submitted
9) Add an email address for your library (in either the email field or the replyto field). Refresh the OPAC problem report page. The form should now show, and the recipient field should say 'library'.
10) Complete the form and submit. Check the message_queue in the database and confirm the to_address is correct. Confirm there is a success message.
11) Add an email address for the syspref KohaAdminEmailAddress and refresh the OPAC problem report page again. The recipient field should now be a dropdown. Select the Koha Administrator option.
12) Complete the form and submit. Check the message_queue in the database and confirm the to_address is the value in KohaAdminEmailAddress. Confirm there is a success message.
13) Ensure all details in the message_queue are correct.
14) Log out of the OPAC
15) Click the Report a problem link again and confirm you are forced to log in

Sponsored-by: Catalyst IT
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 100509 [details] [review]
Bug 4461: (follow-up) Redirect to 404 if OPACProblemReport not enabled

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

Created attachment 100510 [details] [review]
Bug 4461: (follow-up) Creating sample notice and other fixes

- filters
- capitalisation
- atomic update

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

Created attachment 100511 [details] [review]
Bug 4461: Rename report date with created_on

And make it a timestamp type column

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

Created attachment 100512 [details] [review]
Bug 4461: Use KohaAdminEmailAddress pref as 'from'

We usually do not specify the from_address on enqueuing the notice, but
when sending it (from _send_message_by_email). That way a) the code is
centralized (as we must use $patron->notice_email_address) and b) the
email used is the one that is in the DB when the letter is effectively
sent.
Here I guess you need to give it when the notice is enqueued as you want
to default to the koha admin address.

I do not think it's a good idea, we should not send an email with "from"
if not really sent by the user.
We have the borrowernumber anyway to know who filled the form.

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

Created attachment 100513 [details] [review]
Bug 4461: Use our local_referer instead of HTTP_REFERER

Cannot remember why exactly, but it seems that we should not use
HTTP_REFERER directly.

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

Created attachment 100514 [details] [review]
Bug 4461: Redirect to 404 if KohaAdminEmailAddress is not defined

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

Created attachment 100515 [details] [review]
Bug 4461: Simplify recipients code

As the feature is now turned off without KohaAdminEmailAddress, we only
need to check if the email address of the library if defined.
Not that we should not check for $library->branchreplyto or
ReplytoDefault, but only $library->branchemail, as we do everywhere else
(I think)

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

Created attachment 100516 [details] [review]
Bug 4461: Better error handling

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

Created attachment 100517 [details] [review]
Bug 4461: Remove hard requirement on OpacKohaUrl

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

Created attachment 100518 [details] [review]
Bug 4461: Drop 'from_address' use 'reply_address'

This patch adds support for using the reply-to field added in bug 10269
and drops 'from_address' as it will get defined at a later stage in the
emailing process anyway.

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

Created attachment 100519 [details] [review]
Bug 4461: Use get_effective_email from bug 22823

Use the get_effective_email from Koha::Library to get an appropriate
inbound address for the branch as aposed to using branchemail directly.

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

Created attachment 100520 [details] [review]
Bug 4461: Adding user flags for managing OPAC problem reports

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

Created attachment 100521 [details] [review]
Bug 4461: Add problem reports link to mainpage and admin home

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

Created attachment 100522 [details] [review]
Bug 4461: Manage problem reports on the staff client

Test plan:

- Update database and upgrade schema files (if you haven't already).
Restart memcached
- Check your user's permissions and ensure the 'problem_reports'
permission is ticked. Confirm the OPACReportProblem syspref is enabled
- Log into the OPAC and submit a problem report
- Log into the staff client
- You should see a box at the bottom of the main page showing your
pending problem report
- Click the link and confirm it takes you to the new page for managing
problem reports
- Go to Administration
- Confirm you can see a link to 'OPAC problem reports' under the
'Additional parameters' heading
- Click 'OPAC problem reports'
- Confirm your problem report is showing in the table
- Open the OPAC in another tab and submit at least two more problem
reports (so you should have at least three in the table after
refreshing)
- Try the different buttons
    - selecting multiple problem reports and using the big 'mark
    viewed', 'mark closed', 'mark new' buttons. Confirm there are no
    failures and that the number of selected problem reports is correct
    - select all, clear all, hide viewed, hide closed, hide new, show
    all
    - individual 'mark viewed', 'mark closed', 'mark new' buttons for
    each problem report. Confirm the status shows and the correct button
    is disabled while others are enabled
- Confirm the problem page link works as expected

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

Created attachment 100523 [details] [review]
Bug 4461: (follow-up) Add missing relationship accessor

I think this was a simple case of Aliesha missing a file when commiting.

So I could proceed with testing I just quickly re-implimented the patron
relationship accessor.

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

Created attachment 100524 [details] [review]
Bug 4461: DBIC Schema Files

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

This is looking great now Aliesha, thankyou for the addition of a UI, it makes is significantly easier to test and more of a complete feature.

I feel we've hit minimum viable product here now and would love to get this go through.. I added a couple of followups simply to aid in testing.. firstly I think you just missed adding the file to the commit for the patron relationships accessor and then I added a commit with the dbic schema changes so this can be tested on sandboxes and the likes.

Signing off.

(In reply to Martin Renvoize from comment #106)
> This is looking great now Aliesha, thankyou for the addition of a UI, it
> makes is significantly easier to test and more of a complete feature.
> 
> I feel we've hit minimum viable product here now and would love to get this
> go through.. I added a couple of followups simply to aid in testing..
> firstly I think you just missed adding the file to the commit for the patron
> relationships accessor and then I added a commit with the dbic schema
> changes so this can be tested on sandboxes and the likes.
> 
> Signing off.

Thanks I did forget to attach that file!

Created attachment 100619 [details] [review]
Bug 4461: Update license statements

report_problems.borrowernumber defaults to NULL. Should not it be NOT NULL instead?

When the form is sent, the success message appears but with the form bellow.

Should not we hide it?

https://snipboard.io/HJKtdp.jpg

- I would prefer status to be a varchar and have a readable status, instead of N, V, C
- The "hide viewed" and  "hide closed" filters does not work if the page is not reloaded
- Why do not we see the recipient of the report in the table?
We see the reports sent to the administrator, is that expected?
- I think there is a missing test in the controller (minor)
If recipient is forced to "librarian" but the library does not have an email address defined, then the message will be enqueued anyway.

Created attachment 100989 [details] [review]
Bug 4461: Fix status and borrowernumber fields in problem_reports and more

status varchar(6) with readable statuses
borrowernumber not null default 0
hide form if message successfully sent
fixing hide viewed and hide closed filters
adding recipient column

Created attachment 100990 [details] [review]
Bug 4461: Checks on library email before enqueuing letter

Patch no longer applies 8-(:

root@9512ab8ec1bb:koha(bz4461)$ git bz apply 4461
Bug 4461 - Context-sensitive report a problem screen

100506 - Bug 4461: Database and syspref changes
100507 - Bug 4461: Problem Report Koha objects
100508 - Bug 4461: Submit a problem report
100509 - Bug 4461: (follow-up) Redirect to 404 if OPACProblemReport not enabled
100510 - Bug 4461: (follow-up) Creating sample notice and other fixes
100511 - Bug 4461: Rename report date with created_on
100512 - Bug 4461: Use KohaAdminEmailAddress pref as 'from'
100513 - Bug 4461: Use our local_referer instead of HTTP_REFERER
100514 - Bug 4461: Redirect to 404 if KohaAdminEmailAddress is not defined
100515 - Bug 4461: Simplify recipients code
100516 - Bug 4461: Better error handling
100517 - Bug 4461: Remove hard requirement on OpacKohaUrl
100518 - Bug 4461: Drop 'from_address' use 'reply_address'
100519 - Bug 4461: Use get_effective_email from bug 22823
100520 - Bug 4461: Adding user flags for managing OPAC problem reports
100521 - Bug 4461: Add problem reports link to mainpage and admin home
100522 - Bug 4461: Manage problem reports on the staff client
100523 - Bug 4461: (follow-up) Add missing relationship accessor
100524 - Bug 4461: DBIC Schema Files
100619 - Bug 4461: Update license statements
100989 - Bug 4461: Fix status and borrowernumber fields in problem_reports and more
100990 - Bug 4461: Checks on library email before enqueuing letter

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 4461: Database and syspref changes
error: sha1 information is lacking or useless (installer/data/mysql/kohastructure.sql).
error: could not build fake ancestor
Patch failed at 0001 Bug 4461: Database and syspref changes

Created attachment 102188 [details] [review]
Bug 4461: Database and syspref changes

adding problem_reports table and OPACReportProblem syspref

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

Created attachment 102189 [details] [review]
Bug 4461: Problem Report Koha objects

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

Created attachment 102190 [details] [review]
Bug 4461: Submit a problem report

To test:
1) Apply patch, update database, rebuild schema file
2) Restart koha-common and memcached
3) Confirm that your user has an email address.
4) Confirm that your library does NOT have an email address.
5) Confirm that syspref KohaAdminEmailAddress and syspref ReplytoDefault are not filled. Enable the OPACReportProblem syspref.
6) Log into OPAC
7) Click the 'Report a problem' link at the bottom of whatever page
you're on
8) Notice that there is no form is and there is an error message alerting that reports cannot be submitted
9) Add an email address for your library (in either the email field or the replyto field). Refresh the OPAC problem report page. The form should now show, and the recipient field should say 'library'.
10) Complete the form and submit. Check the message_queue in the database and confirm the to_address is correct. Confirm there is a success message.
11) Add an email address for the syspref KohaAdminEmailAddress and refresh the OPAC problem report page again. The recipient field should now be a dropdown. Select the Koha Administrator option.
12) Complete the form and submit. Check the message_queue in the database and confirm the to_address is the value in KohaAdminEmailAddress. Confirm there is a success message.
13) Ensure all details in the message_queue are correct.
14) Log out of the OPAC
15) Click the Report a problem link again and confirm you are forced to log in

Sponsored-by: Catalyst IT
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 102191 [details] [review]
Bug 4461: (follow-up) Redirect to 404 if OPACProblemReport not enabled

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

Created attachment 102192 [details] [review]
Bug 4461: (follow-up) Creating sample notice and other fixes

- filters
- capitalisation
- atomic update

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

Created attachment 102193 [details] [review]
Bug 4461: Rename report date with created_on

And make it a timestamp type column

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

Created attachment 102194 [details] [review]
Bug 4461: Use KohaAdminEmailAddress pref as 'from'

We usually do not specify the from_address on enqueuing the notice, but
when sending it (from _send_message_by_email). That way a) the code is
centralized (as we must use $patron->notice_email_address) and b) the
email used is the one that is in the DB when the letter is effectively
sent.
Here I guess you need to give it when the notice is enqueued as you want
to default to the koha admin address.

I do not think it's a good idea, we should not send an email with "from"
if not really sent by the user.
We have the borrowernumber anyway to know who filled the form.

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

Created attachment 102195 [details] [review]
Bug 4461: Use our local_referer instead of HTTP_REFERER

Cannot remember why exactly, but it seems that we should not use
HTTP_REFERER directly.

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

Created attachment 102196 [details] [review]
Bug 4461: Redirect to 404 if KohaAdminEmailAddress is not defined

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

Created attachment 102198 [details] [review]
Bug 4461: Simplify recipients code

As the feature is now turned off without KohaAdminEmailAddress, we only
need to check if the email address of the library if defined.
Not that we should not check for $library->branchreplyto or
ReplytoDefault, but only $library->branchemail, as we do everywhere else
(I think)

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

Created attachment 102200 [details] [review]
Bug 4461: Better error handling

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

Created attachment 102201 [details] [review]
Bug 4461: Remove hard requirement on OpacKohaUrl

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

Created attachment 102202 [details] [review]
Bug 4461: Drop 'from_address' use 'reply_address'

This patch adds support for using the reply-to field added in bug 10269
and drops 'from_address' as it will get defined at a later stage in the
emailing process anyway.

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

Created attachment 102203 [details] [review]
Bug 4461: Use get_effective_email from bug 22823

Use the get_effective_email from Koha::Library to get an appropriate
inbound address for the branch as aposed to using branchemail directly.

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

Created attachment 102204 [details] [review]
Bug 4461: Adding user flags for managing OPAC problem reports

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

Created attachment 102205 [details] [review]
Bug 4461: Add problem reports link to mainpage and admin home

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

Created attachment 102206 [details] [review]
Bug 4461: Manage problem reports on the staff client

Test plan:

- Update database and upgrade schema files (if you haven't already).
Restart memcached
- Check your user's permissions and ensure the 'problem_reports'
permission is ticked. Confirm the OPACReportProblem syspref is enabled
- Log into the OPAC and submit a problem report
- Log into the staff client
- You should see a box at the bottom of the main page showing your
pending problem report
- Click the link and confirm it takes you to the new page for managing
problem reports
- Go to Administration
- Confirm you can see a link to 'OPAC problem reports' under the
'Additional parameters' heading
- Click 'OPAC problem reports'
- Confirm your problem report is showing in the table
- Open the OPAC in another tab and submit at least two more problem
reports (so you should have at least three in the table after
refreshing)
- Try the different buttons
    - selecting multiple problem reports and using the big 'mark
    viewed', 'mark closed', 'mark new' buttons. Confirm there are no
    failures and that the number of selected problem reports is correct
    - select all, clear all, hide viewed, hide closed, hide new, show
    all
    - individual 'mark viewed', 'mark closed', 'mark new' buttons for
    each problem report. Confirm the status shows and the correct button
    is disabled while others are enabled
- Confirm the problem page link works as expected

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

Created attachment 102207 [details] [review]
Bug 4461: (follow-up) Add missing relationship accessor

I think this was a simple case of Aliesha missing a file when commiting.

So I could proceed with testing I just quickly re-implimented the patron
relationship accessor.

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

Created attachment 102208 [details] [review]
Bug 4461: Update license statements

Created attachment 102209 [details] [review]
Bug 4461: Checks on library email before enqueuing letter

Created attachment 102210 [details] [review]
Bug 4461: DBIC Schema Files

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

Created attachment 102211 [details] [review]
Bug 4461: Fix status and borrowernumber fields in problem_reports and more

status varchar(6) with readable statuses
borrowernumber not null default 0
hide form if message successfully sent
fixing hide viewed and hide closed filters
adding recipient column

I'm now getting this error message when trying to submit the form, no matter what I do:

[#% We really should avoid reaching this! %] Something wrong happened when sending the report. Please contact your library. 

Note: I'm following the test plan in Comment 117

(In reply to David Nind from comment #137)
> I'm now getting this error message when trying to submit the form, no matter
> what I do:
> 
> [#% We really should avoid reaching this! %] Something wrong happened when
> sending the report. Please contact your library. 
> 
> Note: I'm following the test plan in Comment 117

Do you see something relevant in the Koha log file?

I've retested again. Two issues I've identified:

1) If there is no library email address there is no drop down list on the report a problem page, which is the expected behavour. If there is an email address set for the library then there is a drop down list with 'A librarian' and 'Koha administrator' options. Koha administrator works fine, if you select 'A librarian' you get this error message when submitting "[#% We really should avoid reaching this! %] Something wrong happened when sending the report. Please contact your library."

2)In the staff interface the link to the problem page (in the 'Problem page') displays correctly, however the actual link is adding additional information before it, for example:
- Displays as http://kohadev.myDNSname.org:8080/
- Actual link when you try to click on it: 
http://127.0.0.1:8081/cgi-bin/koha/admin/http%3A%2F%2Fkohadev.myDNSname.org%3A8080%2F

Also, the results from Comment 139 were from testing with koha-testing-dock and starting with ku (Koha using MySQL 5.5).

If I test with ku-mdb (MariaDB) then I get different problems. With KohaAdminEmailAddress set and a library email address set I get this error message when submitting a problem report "[#% We really should avoid reaching this! %] Something wrong happened when sending the report. Please contact your library."

Something has happened since I originally tested, as I didn't have this problem then.

This is the error in the plack-error.log after submitting the problem report in the OPAC (using MariaDB):

36 DBD::mysql::st execute failed: Incorrect datetime value: 'current_timestamp()' for column `koha_kohadev`.`problem_reports`.`created_on`     at row 1 [for Statement "INSERT INTO `problem_reports` ( `borrowernumber`, `branchcode`, `content`, `created_on`, `problempage`, `reci    pient`, `title`, `username`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0="51", 1='CPL', 2="Test 145", 3='current_timestamp()    ', 4="http://kohadev.myDNSname.org:8080/", 5="admin", 6="Test 145", 7='koha'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
37 DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect datetime value: 'current_timestamp()' for column `koha_kohadev`.`problem_reports`.    `created_on` at row 1 at /kohadevbox/koha/Koha/Object.pm line 164
38 Something wrong happened when sending the report problem: Invalid value passed, problem_reports.created_on=current_timestamp() expected     type is datetime at /kohadevbox/koha/opac/opac-reportproblem.pl line 131.

Created attachment 102253 [details]
Bug 4461: Error log when using MySQL 5.5

I've attached the error log when testing on koha-testing-docker with MySQL v 5.5 (started with ku) when submitting a problem report when 'A librarian' is selected.

To clarify comments about MySQL and MariaDB, starting koha-testing-docker with:

- ku: Server version = 5.5.62 MySQL Community Server (GPL)

- ku-mdb: Server version = 10.4.12-MariaDB-1:10.4.12+maria~bionic mariadb.org binary distribution

(In reply to David Nind from comment #140)
> This is the error in the plack-error.log after submitting the problem report
> in the OPAC (using MariaDB):
> 
> 36 DBD::mysql::st execute failed: Incorrect datetime value:
> 'current_timestamp()' for column
> `koha_kohadev`.`problem_reports`.`created_on`     at row 1 [for Statement
> "INSERT INTO `problem_reports` ( `borrowernumber`, `branchcode`, `content`,
> `created_on`, `problempage`, `reci    pient`, `title`, `username`) VALUES (
> ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0="51", 1='CPL', 2="Test 145",
> 3='current_timestamp()    ', 4="http://kohadev.myDNSname.org:8080/",
> 5="admin", 6="Test 145", 7='koha'] at
> /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
> 37 DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect datetime value:
> 'current_timestamp()' for column `koha_kohadev`.`problem_reports`.   
> `created_on` at row 1 at /kohadevbox/koha/Koha/Object.pm line 164
> 38 Something wrong happened when sending the report problem: Invalid value
> passed, problem_reports.created_on=current_timestamp() expected     type is
> datetime at /kohadevbox/koha/opac/opac-reportproblem.pl line 131.

This is coming from a bug in master, see bug 25040.

Created attachment 102279 [details] [review]
Bug 4461: get_effective_email has been renamed inbound_email_address

On commit 027051c938924a33a64014794705526ab68351af
   Bug 22823: Rename method with ->inbound_email_address

(In reply to David Nind from comment #141)
> Created attachment 102253 [details]
> Bug 4461: Error log when using MySQL 5.5
> 
> I've attached the error log when testing on koha-testing-docker with MySQL v
> 5.5 (started with ku) when submitting a problem report when 'A librarian' is
> selected.

The important line was the first one:
135 Something wrong happened when sending the report problem: The method Koha::Library->get_effective_email is not covered by tests!

Sorry David I missed that when rebasing the patches.
That should be fixed by the last patch.
I am resetting to NSO, but you will need to apply patch from bug 25040, if not pushed yet.
I do not have time to retest this feature right now, if you face errors again, please to not spend too much time and FQA it immediatly.

Thanks Jonathan for figuring out the problem!

Applying Bug 25040 before applying 4461 fixes the problem.

When someone gets a chance to fix item 2 (pretty minor - but I don't know how to fix the template) from Comment 139 I'll happily sign off.

David

Created attachment 102281 [details] [review]
Bug 4461: Fix OPAC link from staff

QA: We have a security issue here, we should not make this link
clickable from the staff side.

Created attachment 102308 [details] [review]
Bug 4461: Database and syspref changes

adding problem_reports table and OPACReportProblem syspref

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102309 [details] [review]
Bug 4461: Problem Report Koha objects

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102310 [details] [review]
Bug 4461: Submit a problem report

To test:
1) Apply patch, update database, rebuild schema file
2) Restart koha-common and memcached
3) Confirm that your user has an email address.
4) Confirm that your library does NOT have an email address.
5) Confirm that syspref KohaAdminEmailAddress and syspref ReplytoDefault are not filled. Enable the OPACReportProblem syspref.
6) Log into OPAC
7) Click the 'Report a problem' link at the bottom of whatever page
you're on
8) Notice that there is no form is and there is an error message alerting that reports cannot be submitted
9) Add an email address for your library (in either the email field or the replyto field). Refresh the OPAC problem report page. The form should now show, and the recipient field should say 'library'.
10) Complete the form and submit. Check the message_queue in the database and confirm the to_address is correct. Confirm there is a success message.
11) Add an email address for the syspref KohaAdminEmailAddress and refresh the OPAC problem report page again. The recipient field should now be a dropdown. Select the Koha Administrator option.
12) Complete the form and submit. Check the message_queue in the database and confirm the to_address is the value in KohaAdminEmailAddress. Confirm there is a success message.
13) Ensure all details in the message_queue are correct.
14) Log out of the OPAC
15) Click the Report a problem link again and confirm you are forced to log in

Sponsored-by: Catalyst IT
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102311 [details] [review]
Bug 4461: (follow-up) Redirect to 404 if OPACProblemReport not enabled

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102312 [details] [review]
Bug 4461: (follow-up) Creating sample notice and other fixes

- filters
- capitalisation
- atomic update

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102313 [details] [review]
Bug 4461: Rename report date with created_on

And make it a timestamp type column

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102314 [details] [review]
Bug 4461: Use KohaAdminEmailAddress pref as 'from'

We usually do not specify the from_address on enqueuing the notice, but
when sending it (from _send_message_by_email). That way a) the code is
centralized (as we must use $patron->notice_email_address) and b) the
email used is the one that is in the DB when the letter is effectively
sent.
Here I guess you need to give it when the notice is enqueued as you want
to default to the koha admin address.

I do not think it's a good idea, we should not send an email with "from"
if not really sent by the user.
We have the borrowernumber anyway to know who filled the form.

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102315 [details] [review]
Bug 4461: Use our local_referer instead of HTTP_REFERER

Cannot remember why exactly, but it seems that we should not use
HTTP_REFERER directly.

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102316 [details] [review]
Bug 4461: Redirect to 404 if KohaAdminEmailAddress is not defined

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102317 [details] [review]
Bug 4461: Simplify recipients code

As the feature is now turned off without KohaAdminEmailAddress, we only
need to check if the email address of the library if defined.
Not that we should not check for $library->branchreplyto or
ReplytoDefault, but only $library->branchemail, as we do everywhere else
(I think)

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102318 [details] [review]
Bug 4461: Better error handling

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102319 [details] [review]
Bug 4461: Remove hard requirement on OpacKohaUrl

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102320 [details] [review]
Bug 4461: Drop 'from_address' use 'reply_address'

This patch adds support for using the reply-to field added in bug 10269
and drops 'from_address' as it will get defined at a later stage in the
emailing process anyway.

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102321 [details] [review]
Bug 4461: Use get_effective_email from bug 22823

Use the get_effective_email from Koha::Library to get an appropriate
inbound address for the branch as aposed to using branchemail directly.

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102322 [details] [review]
Bug 4461: Adding user flags for managing OPAC problem reports

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102323 [details] [review]
Bug 4461: Add problem reports link to mainpage and admin home

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102324 [details] [review]
Bug 4461: Manage problem reports on the staff client

Test plan:

- Update database and upgrade schema files (if you haven't already).
Restart memcached
- Check your user's permissions and ensure the 'problem_reports'
permission is ticked. Confirm the OPACReportProblem syspref is enabled
- Log into the OPAC and submit a problem report
- Log into the staff client
- You should see a box at the bottom of the main page showing your
pending problem report
- Click the link and confirm it takes you to the new page for managing
problem reports
- Go to Administration
- Confirm you can see a link to 'OPAC problem reports' under the
'Additional parameters' heading
- Click 'OPAC problem reports'
- Confirm your problem report is showing in the table
- Open the OPAC in another tab and submit at least two more problem
reports (so you should have at least three in the table after
refreshing)
- Try the different buttons
    - selecting multiple problem reports and using the big 'mark
    viewed', 'mark closed', 'mark new' buttons. Confirm there are no
    failures and that the number of selected problem reports is correct
    - select all, clear all, hide viewed, hide closed, hide new, show
    all
    - individual 'mark viewed', 'mark closed', 'mark new' buttons for
    each problem report. Confirm the status shows and the correct button
    is disabled while others are enabled
- Confirm the problem page link works as expected

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102325 [details] [review]
Bug 4461: (follow-up) Add missing relationship accessor

I think this was a simple case of Aliesha missing a file when commiting.

So I could proceed with testing I just quickly re-implimented the patron
relationship accessor.

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102326 [details] [review]
Bug 4461: Update license statements

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102327 [details] [review]
Bug 4461: Checks on library email before enqueuing letter

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102328 [details] [review]
Bug 4461: DBIC Schema Files

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102329 [details] [review]
Bug 4461: Fix status and borrowernumber fields in problem_reports and more

status varchar(6) with readable statuses
borrowernumber not null default 0
hide form if message successfully sent
fixing hide viewed and hide closed filters
adding recipient column

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102330 [details] [review]
Bug 4461: get_effective_email has been renamed inbound_email_address

On commit 027051c938924a33a64014794705526ab68351af
   Bug 22823: Rename method with ->inbound_email_address

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 102331 [details] [review]
Bug 4461: Fix OPAC link from staff

QA: We have a security issue here, we should not make this link
clickable from the staff side.

Signed-off-by: David Nind <david@davidnind.com>

Note: Bug 25040 needs to be applied first for this to work correctly (see comment 146).

Created attachment 102358 [details] [review]
Bug 4461: Database and syspref changes

adding problem_reports table and OPACReportProblem syspref

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102359 [details] [review]
Bug 4461: Problem Report Koha objects

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102360 [details] [review]
Bug 4461: Submit a problem report

To test:
1) Apply patch, update database, rebuild schema file
2) Restart koha-common and memcached
3) Confirm that your user has an email address.
4) Confirm that your library does NOT have an email address.
5) Confirm that syspref KohaAdminEmailAddress and syspref ReplytoDefault are not filled. Enable the OPACReportProblem syspref.
6) Log into OPAC
7) Click the 'Report a problem' link at the bottom of whatever page
you're on
8) Notice that there is no form is and there is an error message alerting that reports cannot be submitted
9) Add an email address for your library (in either the email field or the replyto field). Refresh the OPAC problem report page. The form should now show, and the recipient field should say 'library'.
10) Complete the form and submit. Check the message_queue in the database and confirm the to_address is correct. Confirm there is a success message.
11) Add an email address for the syspref KohaAdminEmailAddress and refresh the OPAC problem report page again. The recipient field should now be a dropdown. Select the Koha Administrator option.
12) Complete the form and submit. Check the message_queue in the database and confirm the to_address is the value in KohaAdminEmailAddress. Confirm there is a success message.
13) Ensure all details in the message_queue are correct.
14) Log out of the OPAC
15) Click the Report a problem link again and confirm you are forced to log in

Sponsored-by: Catalyst IT
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102361 [details] [review]
Bug 4461: (follow-up) Redirect to 404 if OPACProblemReport not enabled

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102362 [details] [review]
Bug 4461: (follow-up) Creating sample notice and other fixes

- filters
- capitalisation
- atomic update

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102363 [details] [review]
Bug 4461: Rename report date with created_on

And make it a timestamp type column

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102364 [details] [review]
Bug 4461: Use KohaAdminEmailAddress pref as 'from'

We usually do not specify the from_address on enqueuing the notice, but
when sending it (from _send_message_by_email). That way a) the code is
centralized (as we must use $patron->notice_email_address) and b) the
email used is the one that is in the DB when the letter is effectively
sent.
Here I guess you need to give it when the notice is enqueued as you want
to default to the koha admin address.

I do not think it's a good idea, we should not send an email with "from"
if not really sent by the user.
We have the borrowernumber anyway to know who filled the form.

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102365 [details] [review]
Bug 4461: Use our local_referer instead of HTTP_REFERER

Cannot remember why exactly, but it seems that we should not use
HTTP_REFERER directly.

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102366 [details] [review]
Bug 4461: Redirect to 404 if KohaAdminEmailAddress is not defined

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102367 [details] [review]
Bug 4461: Simplify recipients code

As the feature is now turned off without KohaAdminEmailAddress, we only
need to check if the email address of the library if defined.
Not that we should not check for $library->branchreplyto or
ReplytoDefault, but only $library->branchemail, as we do everywhere else
(I think)

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102368 [details] [review]
Bug 4461: Better error handling

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102369 [details] [review]
Bug 4461: Remove hard requirement on OpacKohaUrl

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102370 [details] [review]
Bug 4461: Drop 'from_address' use 'reply_address'

This patch adds support for using the reply-to field added in bug 10269
and drops 'from_address' as it will get defined at a later stage in the
emailing process anyway.

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102371 [details] [review]
Bug 4461: Use get_effective_email from bug 22823

Use the get_effective_email from Koha::Library to get an appropriate
inbound address for the branch as aposed to using branchemail directly.

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102372 [details] [review]
Bug 4461: Adding user flags for managing OPAC problem reports

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102373 [details] [review]
Bug 4461: Add problem reports link to mainpage and admin home

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102374 [details] [review]
Bug 4461: Manage problem reports on the staff client

Test plan:

- Update database and upgrade schema files (if you haven't already).
Restart memcached
- Check your user's permissions and ensure the 'problem_reports'
permission is ticked. Confirm the OPACReportProblem syspref is enabled
- Log into the OPAC and submit a problem report
- Log into the staff client
- You should see a box at the bottom of the main page showing your
pending problem report
- Click the link and confirm it takes you to the new page for managing
problem reports
- Go to Administration
- Confirm you can see a link to 'OPAC problem reports' under the
'Additional parameters' heading
- Click 'OPAC problem reports'
- Confirm your problem report is showing in the table
- Open the OPAC in another tab and submit at least two more problem
reports (so you should have at least three in the table after
refreshing)
- Try the different buttons
    - selecting multiple problem reports and using the big 'mark
    viewed', 'mark closed', 'mark new' buttons. Confirm there are no
    failures and that the number of selected problem reports is correct
    - select all, clear all, hide viewed, hide closed, hide new, show
    all
    - individual 'mark viewed', 'mark closed', 'mark new' buttons for
    each problem report. Confirm the status shows and the correct button
    is disabled while others are enabled
- Confirm the problem page link works as expected

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102375 [details] [review]
Bug 4461: (follow-up) Add missing relationship accessor

I think this was a simple case of Aliesha missing a file when commiting.

So I could proceed with testing I just quickly re-implimented the patron
relationship accessor.

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102376 [details] [review]
Bug 4461: Update license statements

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102377 [details] [review]
Bug 4461: Checks on library email before enqueuing letter

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102378 [details] [review]
Bug 4461: DBIC Schema Files

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102379 [details] [review]
Bug 4461: Fix status and borrowernumber fields in problem_reports and more

status varchar(6) with readable statuses
borrowernumber not null default 0
hide form if message successfully sent
fixing hide viewed and hide closed filters
adding recipient column

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102380 [details] [review]
Bug 4461: get_effective_email has been renamed inbound_email_address

On commit 027051c938924a33a64014794705526ab68351af
   Bug 22823: Rename method with ->inbound_email_address

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102381 [details] [review]
Bug 4461: Fix OPAC link from staff

QA: We have a security issue here, we should not make this link
clickable from the staff side.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102382 [details] [review]
Bug 4461: Hide link if KohaAdminEmailAddress is not set

opac-reportproblem.pl returns a 404 in that case

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102383 [details] [review]
Bug 4461: dbic schema update

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102384 [details] [review]
Bug 4461: Extend problem_reports.problempage to TEXT

255 chars is not enough if want want to store any kind of URL, for
instance an authorities search can be much longer

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102385 [details] [review]
Bug 4461: Correctly deal with encoding/escaping chars

This patch deals (hopefully) correctly with encoding and escaping chars.
It also remove OPACBaseURL from the url stored in DB, and readd is on
display, to avoid possible attacks.

Test plan:
Go to the authority search
fill term with something hacky
<script>alert('booh!')</script>And Ŝ♥m€ E★tr₳
Search
Click the "Report a problem" link
Fill the form and make sure the url is displayed correctly
submit
Check problem_reports.problempage in DB => Should be correctly displayed
Go to staff interface, "OPAC problem reports"
=> Confirm the link is correctly display
Click it
=> Confirm that you are at the OPAC, and the URL is correct

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102386 [details] [review]
Bug 4461: Use inbound_email_address to know if we display the dropdown list

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Nice work everyone!

Pushed to master for 20.05

feature not backported to 19.11.x

Nice job!