Bug 3669 - Create a template for circ/add_message.pl
Summary: Create a template for circ/add_message.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-25 07:09 UTC by Owen Leonard
Modified: 2017-12-07 22:16 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Adding a message for user now is more integrated in circulation workflow. Add more standard type of notes in the AV 'BOR_NOTES' to use them more.
Version(s) released in:


Attachments
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (6.94 KB, patch)
2016-03-24 00:32 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (7.60 KB, patch)
2016-03-29 00:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (8.30 KB, patch)
2016-03-31 05:34 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (8.47 KB, patch)
2016-03-31 05:53 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (21.71 KB, patch)
2016-04-04 00:33 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (21.03 KB, patch)
2016-04-07 22:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (21.20 KB, patch)
2016-04-08 07:19 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (10.35 KB, patch)
2016-04-11 01:42 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (12.11 KB, patch)
2016-04-29 01:51 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (13.63 KB, patch)
2016-05-09 22:19 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (13.66 KB, patch)
2016-05-10 07:44 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 3669: Remove parameters passed to action of form (4.57 KB, patch)
2016-06-12 10:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar (13.71 KB, patch)
2016-06-21 11:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3669: Remove parameters passed to action of form (4.64 KB, patch)
2016-06-21 11:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3669 [QA Followup] - Give the text area some breathing room (808 bytes, patch)
2016-06-24 13:27 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:13:31 UTC


---- Reported by oleonard@myacpl.org 2009-09-25 19:09:49 ----

Creating a template for add_message.pl as a standalone interface would accomplish a couple of goals:

1. The process of adding a message could be made less dependent on JavaScript. Any link to add a new message could load the page itself if JavaScript wasn't enabled.

2. We could add an "Add message" button to the standard patron toolbar, enabling staff to add a message from anywhere. The "Add message" button could trigger the display of the form when clicked from circulation.pl or could take the user to add_message.pl



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:13 UTC  ---

This bug was previously known as _bug_ 3669 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3669

Actual time not defined. Setting to 0.0
CC member jwagner@ptfs.com does not have an account here

Comment 1 Aleisha Amohia 2016-03-24 00:32:30 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2016-03-24 06:59:30 UTC
I like the feature and it works fine on the Checkout screen.

The new toolbar button appears on other screens as well (e.g. Details or Fines), but nothing happens.
Comment 3 Owen Leonard 2016-03-24 15:12:14 UTC
I think this should be "Failed QA" until a solution is available which makes the button functional on all pages with that toolbar.

Prompted by Bug 16125 I was working on a patch very similar to this one. I can finish it up and post it as an alternative, or drop it if I'm stepping on toes again.
Comment 4 Aleisha Amohia 2016-03-29 00:21:11 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2016-03-29 07:48:21 UTC
Hi Aleisha

The modal opens now fine from all pages with the members toolbar.
However, if I add a message and click "Save", I'm not taken back to the page where I came from (what I would expect), but to the patron's checkout page.

This is due to a redirect in in circ/add_message.pl, line 55 that is hard coded to circulation.pl

It could take in account the referrer - what do you think about?
Comment 6 Aleisha Amohia 2016-03-29 22:50:41 UTC
(In reply to Marc Véron from comment #5)
> Hi Aleisha
> 
> The modal opens now fine from all pages with the members toolbar.
> However, if I add a message and click "Save", I'm not taken back to the page
> where I came from (what I would expect), but to the patron's checkout page.
> 
> This is due to a redirect in in circ/add_message.pl, line 55 that is hard
> coded to circulation.pl
> 
> It could take in account the referrer - what do you think about?

That is a great idea but I'm not really sure how to do it! Any suggestions? Or does anyone else know how?
Comment 7 Marc Véron 2016-03-30 07:38:25 UTC
(In reply to Aleisha Amohia from comment #6)
> (In reply to Marc Véron from comment #5)
> > Hi Aleisha
> > 
> > The modal opens now fine from all pages with the members toolbar.
> > However, if I add a message and click "Save", I'm not taken back to the page
> > where I came from (what I would expect), but to the patron's checkout page.
> > 
> > This is due to a redirect in in circ/add_message.pl, line 55 that is hard
> > coded to circulation.pl
> > 
> > It could take in account the referrer - what do you think about?
> 
> That is a great idea but I'm not really sure how to do it! Any suggestions?
> Or does anyone else know how?

I think you can make use of $input->referer() in circ/add_message.pl
Comment 8 Aleisha Amohia 2016-03-31 05:34:16 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2016-03-31 05:53:43 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-03-31 13:23:58 UTC
Aleisha, new files should be correctly indented (even if it is created with c/p code).

It does not seem to work for me, the messages are not saved if created from another page than circulation.pl.
Moreover the branchcode is not filled and predefined notes do not appear.
Comment 11 M. Tompsett 2016-04-01 05:00:34 UTC
Comment on attachment 49716 [details] [review]
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar

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

::: circ/add_message.pl
@@ +51,4 @@
>      }
>  )->store;
>  
> +my $url = $input->referer();

http://perldoc.perl.org/CGI.html
"referer()
Return the URL of the page the browser was viewing prior to fetching your script. Not available for all browsers."
-- might be a good idea to // "/cgi...$borrowernumber";
Comment 12 Aleisha Amohia 2016-04-04 00:33:23 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2016-04-04 15:46:54 UTC
Overall this looks great. I am curious why you created a new include file for the add message form but put the modal markup in members-toolbar.inc. Why not just put all the markup in members-toolbar.inc?
Comment 14 Aleisha Amohia 2016-04-06 09:11:05 UTC
(In reply to Owen Leonard from comment #13)
> Overall this looks great. I am curious why you created a new include file
> for the add message form but put the modal markup in members-toolbar.inc.
> Why not just put all the markup in members-toolbar.inc?

It was just how I'd seen it done elsewhere so I followed that. I can move it all into members-toolbar tomorrow if you think that would be better.
Comment 15 Aleisha Amohia 2016-04-07 22:21:32 UTC Comment hidden (obsolete)
Comment 16 Marc Véron 2016-04-08 07:19:27 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-04-08 13:35:44 UTC
Comment on attachment 50047 [details] [review]
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar

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

Your patch adds a swp file koha-tmpl/intranet-tmpl/prog/en/includes/.member-add-message.inc.swp

::: members/boraccount.pl
@@ +106,4 @@
>      );
>  }
>  
> +my $branch = C4::Context->userenv->{'branch'};

This value can be retrieved from the templates using LoginBranchcode, it will avoid to pass it from all the scripts.
Keep in mind: if you are duplicating code, you are doing something wrong ;)

@@ +108,5 @@
>  
> +my $branch = C4::Context->userenv->{'branch'};
> +
> +# get authorised values with type of BOR_NOTES
> +my $canned_notes = GetAuthorisedValues("BOR_NOTES");

Same here, the AuthorisedValues template plugin will help you.
Comment 18 Aleisha Amohia 2016-04-11 01:42:04 UTC Comment hidden (obsolete)
Comment 19 Owen Leonard 2016-04-15 15:25:07 UTC
The redirect is failing for me on circulation.pl if I arrive at the page via a form submission. It works if I come to the page via circulation.pl?borrowernumber=X or circulation.pl?findborrower=X.

The same is true for the batch checkout page. If I click the batch checkout link, then try to add a message the redirect works. If I perform a batch checkout and then try to add a message the redirect fails.

In by cases what I mean by "fails" is that I'm returned to a "blank" circulation.pl with no patron loaded.

This is true of paycollect.pl too, so I assume it's true for any page which loads the patron record via a POST operation.
Comment 20 Aleisha Amohia 2016-04-17 22:56:24 UTC
(In reply to Owen Leonard from comment #19)
> The redirect is failing for me on circulation.pl if I arrive at the page via
> a form submission. It works if I come to the page via
> circulation.pl?borrowernumber=X or circulation.pl?findborrower=X.
> 
> The same is true for the batch checkout page. If I click the batch checkout
> link, then try to add a message the redirect works. If I perform a batch
> checkout and then try to add a message the redirect fails.
> 
> In by cases what I mean by "fails" is that I'm returned to a "blank"
> circulation.pl with no patron loaded.
> 
> This is true of paycollect.pl too, so I assume it's true for any page which
> loads the patron record via a POST operation.

I'm a bit confused - the button works fine for me from the paycollect page etc. Not sure what's going wrong for you!
Comment 21 Marc Véron 2016-04-18 07:31:06 UTC
(In reply to Aleisha Amohia from comment #20)
> (In reply to Owen Leonard from comment #19)
> > The redirect is failing for me on circulation.pl if I arrive at the page via
> > a form submission. It works if I come to the page via
> > circulation.pl?borrowernumber=X or circulation.pl?findborrower=X.
> > 
> > The same is true for the batch checkout page. If I click the batch checkout
> > link, then try to add a message the redirect works. If I perform a batch
> > checkout and then try to add a message the redirect fails.
> > 
> > In by cases what I mean by "fails" is that I'm returned to a "blank"
> > circulation.pl with no patron loaded.
> > 
> > This is true of paycollect.pl too, so I assume it's true for any page which
> > loads the patron record via a POST operation.
> 
> I'm a bit confused - the button works fine for me from the paycollect page
> etc. Not sure what's going wrong for you!

To reproduce:
- Go to Batch check out page
- Enter some barcodes, click "Check out"
- On the next page, instead of "Confirm Check out" click "Add message"
- Enter a message and click "Save"

Result: Redirection to /cgi-bin/koha/circ/circulation.pl with text "This patron does not exist. Find another patron?"
Comment 22 Aleisha Amohia 2016-04-29 01:51:36 UTC Comment hidden (obsolete)
Comment 23 Aleisha Amohia 2016-04-29 01:53:11 UTC
(In reply to Marc Véron from comment #21)
> To reproduce:
> - Go to Batch check out page
> - Enter some barcodes, click "Check out"
> - On the next page, instead of "Confirm Check out" click "Add message"
> - Enter a message and click "Save"
> 
> Result: Redirection to /cgi-bin/koha/circ/circulation.pl with text "This
> patron does not exist. Find another patron?"

hi Marc, finally figured out what the problem was - the batch checkout form submits to circulation.pl so the Add message form will always redirect to an empty circulation page. I've given a more specific URL for the form to submit to now. This is the best possible fix I could think of! Unless you can think of a better way?
Comment 24 Marc Véron 2016-04-29 06:43:27 UTC
Hi Aleisha,

I went through the test plan, everything seems to work OK, including returning to Batch checkout (Confirmatoion page, Comment #21).

However, with patch I get an error in the JavaScript console:
ReferenceError: MSG_ADD_MESSAGE is not defined
(...)/intranet-tmpl/prog/en/js/pages/circulation.js
Line: 17

Without patch, the error does not occur.
Comment 25 Aleisha Amohia 2016-05-09 22:19:20 UTC Comment hidden (obsolete)
Comment 26 Marc Véron 2016-05-10 07:44:33 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2016-06-12 10:10:09 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2016-06-12 10:12:28 UTC
It would be nicer to get a message to inform the librarian that the message has been added successfully.
Actually it would have been better to ajaxify this script, to avoid the reload and the problematic of POST form.
Owen, do you think this is an acceptable solution and then we should ajaxify this script?
Comment 29 Katrin Fischer 2016-06-19 19:20:22 UTC
I think this patch is an improvement and seems to work well now as far as I can tell. 

Small glitch with and without patch: if you don't enter a message it takes a while for the page to load, but there is no message saved.
Comment 30 Owen Leonard 2016-06-20 12:14:58 UTC
I agree that this is an improvement and I don't think we should wait for an AJAX version (but it's a good idea).
Comment 31 Jonathan Druart 2016-06-21 11:40:16 UTC
Created attachment 52641 [details] [review]
Bug 3669: Moving 'Add a new message' into a pop up box and adding to patron toolbar

This patch moves all the code into an include which can be accessed from the 'Add a new message' link or the patron toolbar.

EDIT: Ensuring modal opens on all pages
EDIT2: Once the form has been submitted the page will redirect to where you submitted the form from.
EDIT3: Fixing indentation of member-add-message.inc
       Ensuring messages save on every page
       Fill branch
       Ensure predefined notes appear
       Form redirects to where it was submitted from, or just the circulation page.
EDIT4: Removing the new include file and moving it all into the modal in the toolbar
EDIT5: Removing swp file and unnecessary code duplication by utilising plugins
EDIT6: Batch checkout fixes
EDIT7: Ensure you do not get JS error from Comment 24 (Batch checkout syspref must be turned on)

To test:
1) Go to a patron circ page (circ/circulation.pl?borrowernumber=X)
2) Click 'Add a new message' under Messages
3) Confirm this brings up the modal to add a message. Confirm clicking Save saves your message. If it is an OPAC message (for the user), confirm it shows as expected on the OPAC.
4) Click Cancel. Confirm this closes the modal.
5) Click 'Add message' button in toolbar. Complete steps 3 and 4.
6) Confirm the modal opens from all other pages with the members toolbar. Confirm predefined notes shows.
	circ/circulation.pl
	members/moremember.pl
	members/routing-lists.pl
	members/statistics.pl
	members/boraccount.pl
	members/pay.pl
	members/maninvoice.pl
	members/mancredit.pl
	members/readingrec.pl
	members/notices.pl
	members/member-flags.pl
	members/member-password.pl
	members/paycollect.pl
	members/files.pl
        turn on BatchCheckouts syspref and put in appropriate patron category
        circ/circulation.pl?borrowernumber=X&batch=1
7) Confirm that the page redirects to where you submitted the form from once you have submitted it.
8) Go to Check Out tab and confirm your message saved (or OPAC, wherever you saved it)

Sponsored-by: Catalyst IT

Followed test plan, works as expected. (Re-tested for comment #24)
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Jonathan Druart 2016-06-21 11:40:20 UTC
Created attachment 52642 [details] [review]
Bug 3669: Remove parameters passed to action of form

Sounds weird to pass parameters to the action of a POST form.
This patch adds a workaround.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 33 Jonathan Druart 2016-06-21 11:44:14 UTC
(In reply to Katrin Fischer from comment #29)
> Small glitch with and without patch: if you don't enter a message it takes a
> while for the page to load, but there is no message saved.

I don't notice that (under plack).
Comment 34 Katrin Fischer 2016-06-21 15:38:11 UTC
Was thinking maybe the field should be 'required' somehow - but it's not a blocker :)
Comment 35 Kyle M Hall 2016-06-24 13:27:14 UTC
Created attachment 52796 [details] [review]
Bug 3669 [QA Followup] - Give the text area some breathing room

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 36 Kyle M Hall 2016-06-24 13:28:41 UTC
Pushed to master for 16.11, thanks Aleisha, Jonathan!