Bug 12029 - Patrons should be able to delete their patron messages
Summary: Patrons should be able to delete their patron messages
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
: 33442 (view as bug list)
Depends on:
Blocks: 33097
  Show dependency treegraph
 
Reported: 2014-04-02 18:32 UTC by Christopher Brannon
Modified: 2023-12-28 20:43 UTC (History)
14 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact: Caroline Cyr La Rose
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/687
Text to go in the release notes:
This enhancement adds the ability for patrons to dismiss an OPAC message, marking it as read to remove it from their summary page.
Version(s) released in:
23.05.00


Attachments
Bug 12029: Patrons should be able to delete their patron messages (12.99 KB, patch)
2022-02-11 16:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 12029: Patrons should be able to delete their patron messages (13.05 KB, patch)
2022-02-11 17:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12029: Patrons should be able to delete their patron messages (13.09 KB, patch)
2022-02-11 18:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12029: (follow-up) Adhering to coding guidelines (6.81 KB, patch)
2022-04-04 02:03 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 12029: (follow-up) Adhering to coding guidelines (6.86 KB, patch)
2022-04-04 18:14 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 12029: Patrons should be able to delete their patron messages (12.89 KB, patch)
2022-07-08 18:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 12029: (follow-up) Adhering to coding guidelines (7.52 KB, patch)
2022-07-08 18:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 12029: Patrons should be able to delete their patron messages (12.89 KB, patch)
2022-07-11 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12029: (follow-up) Adhering to coding guidelines (7.52 KB, patch)
2022-07-11 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12029: DBIC Schema (1.44 KB, patch)
2022-07-11 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12029: Patrons should be able to delete their patron messages (12.96 KB, patch)
2022-07-11 14:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12029: (follow-up) Adhering to coding guidelines (7.58 KB, patch)
2022-07-11 14:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12029: DBIC Schema (1.50 KB, patch)
2022-07-11 14:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12029: Patrons should be able to delete their patron messages (13.02 KB, patch)
2022-07-12 18:18 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 12029: (follow-up) Adhering to coding guidelines (7.64 KB, patch)
2022-07-12 18:18 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 12029: DBIC Schema (1.56 KB, patch)
2022-07-12 18:18 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 12029: Adding messages.patron_read_date column (2.12 KB, patch)
2023-02-16 02:27 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12029: Schema updates (1.39 KB, patch)
2023-02-16 02:27 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12029: Tests (1.99 KB, patch)
2023-02-16 02:27 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12029: Ability for patrons to dismiss OPAC messages (8.14 KB, patch)
2023-02-16 02:27 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12029: Ability for patrons to dismiss OPAC messages (9.71 KB, patch)
2023-02-28 21:01 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12029: Adding messages.patron_read_date column (2.16 KB, patch)
2023-02-28 21:24 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12029: Schema updates (1.44 KB, patch)
2023-02-28 21:24 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12029: Tests (2.05 KB, patch)
2023-02-28 21:24 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12029: Ability for patrons to dismiss OPAC messages (9.77 KB, patch)
2023-02-28 21:24 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12029: Adding messages.patron_read_date column (2.30 KB, patch)
2023-03-23 13:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12029: Schema updates (1.52 KB, patch)
2023-03-23 13:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12029: Tests (2.13 KB, patch)
2023-03-23 13:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12029: Ability for patrons to dismiss OPAC messages (9.86 KB, patch)
2023-03-23 13:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12029: Remove 'params' from filter_by_unread (2.14 KB, patch)
2023-03-23 13:02 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2014-04-02 18:32:45 UTC
It would be great if patrons could delete private messages displayed to them in the OPAC.  Right now, if staff use this feature, patrons have to remind staff to remove the message.

Christopher
Comment 1 George Williams (NEKLS) 2021-07-14 16:02:42 UTC
We have AllowAllMessageDeletion set to "Don't allow" so I'd like to see a system preference that can turn this feature on or off if added.
Comment 2 Christopher Brannon 2021-08-13 15:42:21 UTC
(In reply to George Williams (NEKLS) from comment #1)
> We have AllowAllMessageDeletion set to "Don't allow" so I'd like to see a
> system preference that can turn this feature on or off if added.

After reviewing the intended functionality of this feature and the purpose of AllowAllMessageDeletion, we have determined that this feature will not change anything with this permission.
Comment 3 Christopher Brannon 2021-08-13 15:57:17 UTC
The koha-US development committee met and has determined the following criteria for this new feature:

* A new field (ie patron_read) will be added to the messages table to record that the patron has read the message.

* If a message is set to display on the OPAC for the patron, the patron will have a 'Dismiss' or 'Mark as read' button next to the message.

* When the patron marks the message as read, the message_type field will change from B (patron) to L (librarian), and the new field (ie patron_read) will have a value, indicating that they read the message.

* If this new field is true (showing the patron has read the message), it will now have the message_type set to L (librarian), and the message will show on the patron account.  Because the new field has a value, it will also display some form of label next to the message indicating that the patron has read the message.  The label could say something like 'Patron acknowledged', or something similar.

This functionality will not delete the message.  It will allow staff to see that the patron has read the message, and staff will still have the ability to delete the message at their discretion.
Comment 4 Autoparallel 2021-08-13 17:19:35 UTC
Since this is what came out from the committee we will consider it as the official scope for this project and what will be built.

Please be aware that any changes to this scope will have to be re-discussed in the committee and if approved would have to be treated as a new development, which would require a new work agreement.
Comment 5 Katrin Fischer 2021-08-16 13:18:04 UTC
Probably should stay away... but since this question could also come up later in the process...: Why the change from B to L? For me that indicates for whose eyes the message was initially intended for and I think changing it, might make us lose some valuable information. Reporting could for example be affected. Visibility to the patron in the OPAC and also the new note "patron acknowledged" could also use just rely on the new db column. So the change B/L would not be strictly necessary to achieve the described behaviour.
Comment 6 Owen Leonard 2021-08-16 13:36:59 UTC
(In reply to Katrin Fischer from comment #5)
> Why the change from B to L?

I agree that this doesn't seem to be a necessary step.
Comment 7 Christopher Brannon 2021-08-16 15:10:15 UTC
(In reply to Owen Leonard from comment #6)
> (In reply to Katrin Fischer from comment #5)
> > Why the change from B to L?
> 
> I agree that this doesn't seem to be a necessary step.

This step was added because the message system does not archive these messages.  If the message were just deleted, there would be no easy way for staff to see that the patron acknowledged receiving the message.  If it were marked and hidden, we would be retaining messages that would build up over time, and the table would have to be maintained.

When a message is marked as B (patron), it is seen on BOTH sides, by patron and staff.  Changing it to L (staff) would accomplish dismissing it on the patron side, and maintaining current workflows on the staff side.  After this enhancement, staff would be able to KNOW that the patron has seen the message, and the staff could decide whether to keep or delete the message from that point.  Right now, staff have NO idea if the message is seen, and therefore have no idea when it would be appropriate to delete, if desired.

This workflow enhancement has the least impact on current practices while adding needed functionality, and adding information for staff to enhance their workflow with these messages.
Comment 8 Christopher Brannon 2021-08-16 15:12:13 UTC
One small adjustment I would suggest, and should be easy enough, is to add the date of when the message was read to the label on the staff side.  Perhaps the date could be the flag in the new field.
Comment 9 Owen Leonard 2021-08-16 16:32:28 UTC
(In reply to Christopher Brannon from comment #7)
 
> When a message is marked as B (patron), it is seen on BOTH sides, by patron
> and staff.  Changing it to L (staff) would accomplish dismissing it on the
> patron side, and maintaining current workflows on the staff side.

It's not necessary if we're using logic in the OPAC to hide any message which is marked as seen.
Comment 10 Christopher Brannon 2021-08-16 16:46:19 UTC
(In reply to Owen Leonard from comment #9)
> (In reply to Christopher Brannon from comment #7)
>  
> > When a message is marked as B (patron), it is seen on BOTH sides, by patron
> > and staff.  Changing it to L (staff) would accomplish dismissing it on the
> > patron side, and maintaining current workflows on the staff side.
> 
> It's not necessary if we're using logic in the OPAC to hide any message
> which is marked as seen.

Oh, wait.  I think I understand.  So, if we flag the it as read by putting a date in the new field, we just turn off visibility on the opac side.  We don't have to touch the message_type field at all.  That makes sense.  :)  Am I understanding correctly?  Everything else should still work the same, because we will still be able to see it on the staff side, and it will still be flagged as read?
Comment 11 Katrin Fischer 2021-08-16 17:02:16 UTC
> Oh, wait.  I think I understand.  So, if we flag the it as read by putting a
> date in the new field, we just turn off visibility on the opac side.  We
> don't have to touch the message_type field at all.  That makes sense.  :) 
> Am I understanding correctly?  Everything else should still work the same,
> because we will still be able to see it on the staff side, and it will still
> be flagged as read?

Yep, exactly what I meant. This way we don't change the current logic, but just enhance it a bit further. Changing the display logic in the OPAC to filter by the new 'read flag' should not be too hard and it will be a little less disruptive for the current workflows. Also, if we needed to revert from read to unread or wanted to optionally show read messages again, that would be relatively easy, because we can still tell what messages are 'patron' messages. 

I like the idea of making the 'read flag' a date.
Comment 12 Christopher Brannon 2021-08-16 17:22:05 UTC
Okay, based on this discussion, we can simplify the scope of this project:

* A new field (ie patron_read) will be added to the messages table to record that the patron has read the message.

* If a message is set to display on the OPAC for the patron, the patron will have a 'Dismiss' or 'Mark as read' button next to the message.

* When the patron marks the message as read, the new field (ie patron_read) will have the current date added to it, indicating the date that they read the message.

* If this new field has a value (a date), it will no longer show on the OPAC.  It will also display a label, such as 'Read xx-xx-xxxx', where x is the date in the field.  (Date will need to be formatted to admin date settings.)

This functionality will not delete the message.  It will allow staff to see that the patron has read the message, and staff will still have the ability to delete the message at their discretion.
Comment 13 Christopher Brannon 2021-10-15 19:58:34 UTC
Updated per Auto-Parallel's request:

* A new field (ie patron_read) will be added to the messages table to record that the patron has read the message.

* If a message is set to display on the OPAC for the patron, the patron will have a 'Dismiss' or 'Mark as read' button next to the message.

* When the patron marks the message as read, the new field (ie patron_read) will have the current date added to it, indicating the date that they read the message.

* If this new field "patron_read" has a value (a date), the dismissed message will no longer show on the patron user interface (OPAC) only. In the staff user interface, the dismissed message will still be visible and will also display a label, such as 'Read xx-xx-xxxx', where x is the date in the field. (Date will need to be formatted to admin date settings.)

This functionality will not delete the message.  It will allow staff to see that the patron has read the message, and staff will still have the ability to delete the message at their discretion.
Comment 14 George Williams (NEKLS) 2021-10-15 19:59:47 UTC
(In reply to Christopher Brannon from comment #13)
> Updated per Auto-Parallel's request:
> 
> * A new field (ie patron_read) will be added to the messages table to record
> that the patron has read the message.
> 
> * If a message is set to display on the OPAC for the patron, the patron will
> have a 'Dismiss' or 'Mark as read' button next to the message.
> 
> * When the patron marks the message as read, the new field (ie patron_read)
> will have the current date added to it, indicating the date that they read
> the message.
> 
> * If this new field "patron_read" has a value (a date), the dismissed
> message will no longer show on the patron user interface (OPAC) only. In the
> staff user interface, the dismissed message will still be visible and will
> also display a label, such as 'Read xx-xx-xxxx', where x is the date in the
> field. (Date will need to be formatted to admin date settings.)
> 
> This functionality will not delete the message.  It will allow staff to see
> that the patron has read the message, and staff will still have the ability
> to delete the message at their discretion.

This is an excellent enhancement to the development.
Comment 15 Owen Leonard 2022-02-11 16:59:11 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2022-02-11 17:19:57 UTC Comment hidden (obsolete)
Comment 17 Lisette Scheer 2022-02-11 17:21:35 UTC
Worked great. I also ran a report:
SELECT *
FROM messages
WHERE borrowernumber="51"

and the patron_read_date was included: 2022-02-11 17:16:58.

I know we have plenty of patrons who will be excited about this.
Comment 18 Kyle M Hall 2022-02-11 18:23:49 UTC
Created attachment 130516 [details] [review]
Bug 12029: Patrons should be able to delete their patron messages

This patch adds an option for patrons to dismiss OPAC messages sent to
them by the staff. The messages remain visible in the staff interface
with an indication that the message has been read.

To test, apply the patch and run the database update.

- View a patron record in the staff interface.
- Click "Add message" and select "OPAC" from the "Add a message for..."
  dropdown.
- Add a message.

- Log in to the OPAC as that patron.
- On the "Your summary" page you should see the message along with a
  "Dismiss" link."
- Clicking the "Dismiss" link should make the message disappear.
- Go to the OPAC home page. The "User summary" box in the right sidebar
  should have the correct count of messages.

- View the patron record in the staff interface again.
- The list of patron messages should still include the message which was
  dismissed via the OPAC, but there should be indication that the
  message was read, e.g. "read 11-2-2022"

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jason Robb <jrobb@sekls.org>
Comment 19 Jonathan Druart 2022-02-18 10:52:05 UTC
1. Empty new line at the top of Koha/Patron/Messages.pm

2. Koha::Patron::Messages->unread should be ->filter_by_unread

3. You don't need unread_count, just use ->filter_by_unread->count

4. Missing tests for this new method

5. DBIC schema change should not be included in the patch

6. atomicupdate file has a qq{} but you don't need string interpolation (ie. replace with q{}

7. atomicupdate is too verbose, but RM can deal with that

8. Wrong copyright statement in opac/dismiss_message.pl (the one from 2009)

9. Unexpected comment in opac/dismiss_message.pl

    # no warnings qw/once/;
    # my $ig = 'Examine variable $exception';
    # push @DB::typeahead, 'x $exception';
    # $DB::single++;

    # uncomment assignment below if you want to view environment
    # $result->{env} = \%ENV;

10. It would have been better to write a ajax script, like opac/svc/checkout_notes

11. What are you doing with $result? $inputs and comment are returned from the dismiss_patron_message sub but never reused. This script could be 10 lines long IMO.
Comment 20 Katrin Fischer 2022-02-19 00:01:58 UTC
Hi Bill, thx for providing a patch! 

Jonathan provided you some good feedback on things that need to be fixed, please let us know if something is not clear!
Comment 21 Martin Renvoize 2022-02-24 10:06:49 UTC
Welcome Bill,

This looks like a great new feature and I'm really glad the discussion continued past comment 4 to get the refinements I also felt were necessary.

Regarding the actual submission, did you read the coding guidelines available https://wiki.koha-community.org/wiki/Coding_Guidelines ?

These are what the QA people will base their most basic feedback upon and are 'hard fails' in most cases when not adhered to. They are voted upon by a council of community members and have good reasoning behind each (though they do not cover all cases and being updated fairly regularly as we come across various patterns.. for exapme the 'filter_by_' requirement is not yet a formal guideline, but will become one very soon)

Beyond hard fails, the QA persons role is to help spot regressions and ensure coding of a minimum standard is maintained for submissions so we can maintain the features going forward.  Such advice can be argued against, but should generally be considered constructive.

So, taking Jonathans list.

1. Style, this may fail guidelines given our perlcritic requirements. It's a 1s fix anyway.
2. Will be in the guidelines imminently, a few minutes to fix.
3. Style, improved coding style, readability improvement. Not a hard fail, but constructive.
4. Hard fail, all module changes have required corresponding unit tests since 2017. If you are stuck here, just ask for some guidance. https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL17:_Unit_tests_are_required_.28updated_Apr_26.2C_2017.29
5. This is an unwritten rule.. I've made a note to add it to the guidelines.  It just makes submissions considerably simpler for maintainers to add to all the supported branches.
6. Style, no guidelines broken, but it's a fair comment to help improve the developer. Another 10s fix.
7. Style, this will be fixed at push.
8. Hard fail: https://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
9. Superfluous comments confuse future maintenance and cost everybody time.
10. Style, developers do things in their own way. Whilst I agree an ajax method would have been preferable (I'd probably have gone for a new REST route myself, rather than svc), this isn't a hard fail but more of a guide for future developments.
11. This is highlighting some red flags for future maintenance again. This is the most 'human' comment here. To me, this code looks rather 'copy/paste', which is fine but there should be evidence that you've stripped out all unnecessary boilerplate and given the development some thought as to what it's actually doing and requires.

Hope that helps clarify things.
Comment 22 Martin Renvoize 2022-02-24 10:18:18 UTC
Comment on attachment 130516 [details] [review]
Bug 12029: Patrons should be able to delete their patron messages

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

::: installer/data/mysql/kohastructure.sql
@@ +3739,4 @@
>    `message` mediumtext COLLATE utf8mb4_unicode_ci NOT NULL COMMENT 'the text of the message',
>    `message_date` timestamp NOT NULL DEFAULT current_timestamp() COMMENT 'the date and time the message was written',
>    `manager_id` int(11) DEFAULT NULL COMMENT 'creator of message',
> +  `patron_read_date` timestamp NULL DEFAULT NULL COMMENT 'date and time the patron dismissed the message',

Final additional note from my own review:

Shouldn't this be a DATETIME rather than a TIMESTAMP field?
Comment 23 Hayley Pelham 2022-04-04 02:03:17 UTC Comment hidden (obsolete)
Comment 24 Hayley Pelham 2022-04-04 02:06:24 UTC
(In reply to Martin Renvoize from comment #22)
> Comment on attachment 130516 [details] [review] [review]
> Bug 12029: Patrons should be able to delete their patron messages
> 
> Review of attachment 130516 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: installer/data/mysql/kohastructure.sql
> @@ +3739,4 @@
> >    `message` mediumtext COLLATE utf8mb4_unicode_ci NOT NULL COMMENT 'the text of the message',
> >    `message_date` timestamp NOT NULL DEFAULT current_timestamp() COMMENT 'the date and time the message was written',
> >    `manager_id` int(11) DEFAULT NULL COMMENT 'creator of message',
> > +  `patron_read_date` timestamp NULL DEFAULT NULL COMMENT 'date and time the patron dismissed the message',
> 
> Final additional note from my own review:
> 
> Shouldn't this be a DATETIME rather than a TIMESTAMP field?

As the similar field message_date is also a timestamp I see no need to change this aspect of the patch at this stage. Please let me know if you have other reasoning!
Comment 25 Owen Leonard 2022-04-04 18:14:08 UTC
Created attachment 132949 [details] [review]
Bug 12029: (follow-up) Adhering to coding guidelines

This is a follow-up patch which (based on comment #19) does
the following:

1) Removes newline from top of Koha/Patron/Messages.pm
2) Changes method Koha::Patron::Messages->unread to ->filter_by_unread
3) Removes method Koha::Patron::Messages->unread_count
4) (Will address tests in separate patch)
5) Removes changes to Koha/Schema/Result/Message.pm
6) Changes atomicupdate from qq{} to q{}
7) (Left for RM)
8) Removes extra copyright statement in opac/dismiss_message.pl
9) Removes extra comments from opac/dismiss_message.pl
10) (Left for now)
11) (Left for now)

Sponsored-by: Horowhenua Library Trust

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 26 Owen Leonard 2022-04-04 18:15:53 UTC
Thank you for the follow-up Hayley!
Comment 27 Katrin Fischer 2022-04-25 09:27:05 UTC
Hi Hayley and all, 

trying to give this another push forward.

1) Unit tests (blocker)
We are still missing the unit tests. Is there any update on that?

2) Database update
The database update is still a bit too verbose, as noted earlier, using some uncommon output pattern (ALTERATION, NOTICE and INFO are not needed). We said RM could deal with it, so just explaining a bit more what people meant earlier on:

+use Modern::Perl;
+
+return {
+    bug_number => "12029",
+    description => "Enable patrons to delete messages",
+    up => sub {
+        my ($args) = @_;
+        my ($dbh, $out) = @$args{qw(dbh out)};
+
+        # Do you stuffs here
+        my $alteration = q{
+            ALTER TABLE messages
+            ADD COLUMN `patron_read_date` timestamp NULL DEFAULT NULL
+            COMMENT 'date and time patron dismissed message'
+            AFTER `manager_id`
+        };
+        if( column_exists('messages', 'patron_read_date') ) {
+            say $out "NOTICE: Column 'messages.patron_read_date' already exists";
+        }
+        else {
+            say $out "ALTERATION: $alteration";
+            $dbh->do($alteration);
+        }
+
+        # Print useful stuff here
+        say $out "INFO: Bug 12029 migration applied";
+    },
+};

3) Translatability

There is some code in opac/dismiss_message.pl that I wondered about. It contains messages that are not translatable, but I am not sure yet if this would show to patrons, in my tests it didn't seem like it.

comment => "Patron read date already set",
comment => "Set patron_read_date to ($patron_read_date)",

Hm, they might not be used after all?

>11. What are you doing with $result? $inputs and comment are returned from the dismiss_patron_message sub but never >reused. This script could be 10 lines long IMO.


Failing for 1). Testing in the GUI worked well!
Comment 28 Christopher Brannon 2022-04-25 14:46:20 UTC
Auto-Parallel, who wrote the patch, is supposed to be uploading the unit tests.  We are waiting for them to do this.
Comment 29 Katrin Fischer 2022-04-25 15:01:03 UTC
Thx for the update, Christopher. 

If we could get the unit tests within the next few days, we still might make the release.
Comment 30 Owen Leonard 2022-07-08 18:53:12 UTC Comment hidden (obsolete)
Comment 31 Owen Leonard 2022-07-08 18:53:16 UTC Comment hidden (obsolete)
Comment 32 Owen Leonard 2022-07-08 18:56:21 UTC
FYI I've changed the assignment to myself because Autoparallel had no interest in participating in the Bugzilla process. I've attached the last version of their patch and what I think is a mangled version of Hayley's followup. Everything seems to work correctly but I'm going to be cautious by asking for new signoffs.
Comment 33 Christopher Brannon 2022-07-08 20:25:24 UTC Comment hidden (obsolete)
Comment 34 Martin Renvoize 2022-07-11 14:48:23 UTC
Created attachment 137518 [details] [review]
Bug 12029: Patrons should be able to delete their patron messages

This patch adds an option for patrons to dismiss OPAC messages sent to
them by the staff. The messages remain visible in the staff interface
with an indication that the message has been read.

To test, apply the patch and run the database update.

- View a patron record in the staff interface.
- Click "Add message" and select "OPAC" from the "Add a message for..."
  dropdown.
- Add a message.

- Log in to the OPAC as that patron.
- On the "Your summary" page you should see the message along with a
  "Dismiss" link."
- Clicking the "Dismiss" link should make the message disappear.
- Go to the OPAC home page. The "User summary" box in the right sidebar
  should have the correct count of messages.

- View the patron record in the staff interface again.
- The list of patron messages should still include the message which was
  dismissed via the OPAC, but there should be indication that the
  message was read, e.g. "read 11-2-2022"
Comment 35 Martin Renvoize 2022-07-11 14:48:32 UTC
Created attachment 137519 [details] [review]
Bug 12029: (follow-up) Adhering to coding guidelines

This is a follow-up patch which (based on comment #19) does
the following:

1) Removes newline from top of Koha/Patron/Messages.pm
2) Changes method Koha::Patron::Messages->unread to ->filter_by_unread
3) Removes method Koha::Patron::Messages->unread_count
4) (Will address tests in separate patch)
5) Removes changes to Koha/Schema/Result/Message.pm
6) Changes atomicupdate from qq{} to q{}
7) (Left for RM)
8) Removes extra copyright statement in opac/dismiss_message.pl
9) Removes extra comments from opac/dismiss_message.pl
10) (Left for now)
11) (Left for now)

Sponsored-by: Horowhenua Library Trust
Comment 36 Martin Renvoize 2022-07-11 14:48:38 UTC
Created attachment 137520 [details] [review]
Bug 12029: DBIC Schema
Comment 37 Martin Renvoize 2022-07-11 14:49:50 UTC
I've added the Schema patch here to make this easier to test on sandboxes...


However.. please note when you see an error like this you can actually build the schema's yourself from the sandbox action menu 'Refresh schema'
Comment 38 Martin Renvoize 2022-07-11 14:58:10 UTC
Created attachment 137521 [details] [review]
Bug 12029: Patrons should be able to delete their patron messages

This patch adds an option for patrons to dismiss OPAC messages sent to
them by the staff. The messages remain visible in the staff interface
with an indication that the message has been read.

To test, apply the patch and run the database update.

- View a patron record in the staff interface.
- Click "Add message" and select "OPAC" from the "Add a message for..."
  dropdown.
- Add a message.

- Log in to the OPAC as that patron.
- On the "Your summary" page you should see the message along with a
  "Dismiss" link."
- Clicking the "Dismiss" link should make the message disappear.
- Go to the OPAC home page. The "User summary" box in the right sidebar
  should have the correct count of messages.

- View the patron record in the staff interface again.
- The list of patron messages should still include the message which was
  dismissed via the OPAC, but there should be indication that the
  message was read, e.g. "read 11-2-2022"

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2022-07-11 14:58:15 UTC
Created attachment 137522 [details] [review]
Bug 12029: (follow-up) Adhering to coding guidelines

This is a follow-up patch which (based on comment #19) does
the following:

1) Removes newline from top of Koha/Patron/Messages.pm
2) Changes method Koha::Patron::Messages->unread to ->filter_by_unread
3) Removes method Koha::Patron::Messages->unread_count
4) (Will address tests in separate patch)
5) Removes changes to Koha/Schema/Result/Message.pm
6) Changes atomicupdate from qq{} to q{}
7) (Left for RM)
8) Removes extra copyright statement in opac/dismiss_message.pl
9) Removes extra comments from opac/dismiss_message.pl
10) (Left for now)
11) (Left for now)

Sponsored-by: Horowhenua Library Trust
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Martin Renvoize 2022-07-11 14:58:23 UTC
Created attachment 137523 [details] [review]
Bug 12029: DBIC Schema

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 41 Martin Renvoize 2022-07-11 14:59:04 UTC
All appears to work as designed.. signing off, ready for QA :)
Comment 42 Christopher Brannon 2022-07-11 15:46:57 UTC
(In reply to Martin Renvoize from comment #37)
> I've added the Schema patch here to make this easier to test on sandboxes...
> 
> 
> However.. please note when you see an error like this you can actually build
> the schema's yourself from the sandbox action menu 'Refresh schema'

I had no idea that I needed to do that.  How can I tell?
Comment 43 Martin Renvoize 2022-07-12 07:18:43 UTC
(In reply to Christopher Brannon from comment #42)
> I had no idea that I needed to do that.  How can I tell?

It's not entirely trivial to tell.. it's either a good habit to get into to just run the schema rebuild after applying patches (this is something I've considered doing by default on the sandboxes, but never had a moment to implement).. or if you look at the logs you posted the `The method Some::Koha::Object->some_method_name is not covered by tests!` line is an indication that it's probably schema build related.
Comment 44 ByWater Sandboxes 2022-07-12 18:18:20 UTC
Created attachment 137646 [details] [review]
Bug 12029: Patrons should be able to delete their patron messages

This patch adds an option for patrons to dismiss OPAC messages sent to
them by the staff. The messages remain visible in the staff interface
with an indication that the message has been read.

To test, apply the patch and run the database update.

- View a patron record in the staff interface.
- Click "Add message" and select "OPAC" from the "Add a message for..."
  dropdown.
- Add a message.

- Log in to the OPAC as that patron.
- On the "Your summary" page you should see the message along with a
  "Dismiss" link."
- Clicking the "Dismiss" link should make the message disappear.
- Go to the OPAC home page. The "User summary" box in the right sidebar
  should have the correct count of messages.

- View the patron record in the staff interface again.
- The list of patron messages should still include the message which was
  dismissed via the OPAC, but there should be indication that the
  message was read, e.g. "read 11-2-2022"

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

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 45 ByWater Sandboxes 2022-07-12 18:18:24 UTC
Created attachment 137647 [details] [review]
Bug 12029: (follow-up) Adhering to coding guidelines

This is a follow-up patch which (based on comment #19) does
the following:

1) Removes newline from top of Koha/Patron/Messages.pm
2) Changes method Koha::Patron::Messages->unread to ->filter_by_unread
3) Removes method Koha::Patron::Messages->unread_count
4) (Will address tests in separate patch)
5) Removes changes to Koha/Schema/Result/Message.pm
6) Changes atomicupdate from qq{} to q{}
7) (Left for RM)
8) Removes extra copyright statement in opac/dismiss_message.pl
9) Removes extra comments from opac/dismiss_message.pl
10) (Left for now)
11) (Left for now)

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

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 46 ByWater Sandboxes 2022-07-12 18:18:28 UTC
Created attachment 137648 [details] [review]
Bug 12029: DBIC Schema

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

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 47 Christopher Brannon 2022-07-12 18:19:40 UTC
Looks good to me as well.
Comment 48 Jonathan Druart 2022-07-13 07:25:43 UTC
DB naming (suffix _date), see bug 31150.
Comment 49 Jonathan Druart 2022-07-13 07:49:00 UTC
There is NO authentication check, everybody (no need to be loggedin) can delete a message. This is a hard fail.

Really, it would have been better (and easier for everybody) to follow the usual pattern we have in our scripts, than decide on something that is not Koha-styled.
Comment 50 Jonathan Druart 2022-07-13 07:51:25 UTC
+    my $borrowernumber = $cgi->param('borrowernumber') || undef;

$borrowernumber MUST be retrieved from get_template_and_user (or another C4::Auth method), not from the query parameters.
Comment 51 Martin Renvoize 2022-07-13 08:43:07 UTC
Must admit, I didn't do a code review here.. just confirmed the feature worked as described.

As you've adopted this one Owen, feel free to ask for help to update it to more usual practices.. I'm happy to help.
Comment 52 Aleisha Amohia 2023-02-16 02:27:27 UTC
Created attachment 146706 [details] [review]
Bug 12029: Adding messages.patron_read_date column
Comment 53 Aleisha Amohia 2023-02-16 02:27:31 UTC
Created attachment 146707 [details] [review]
Bug 12029: Schema updates
Comment 54 Aleisha Amohia 2023-02-16 02:27:35 UTC
Created attachment 146708 [details] [review]
Bug 12029: Tests
Comment 55 Aleisha Amohia 2023-02-16 02:27:39 UTC
Created attachment 146709 [details] [review]
Bug 12029: Ability for patrons to dismiss OPAC messages

This enhancement adds the ability for patrons to dismiss an OPAC
message, marking it as read to remove it from their summary page.

To test:
1) Update database and restart services
2) Log into the staff interface and go to your patron account
3) Click the Add message button
4) Add a message for the OPAC and Save
5) Log into the OPAC. Note there is a message on the homepage saying you
have a message. Go to your user summary and confirm the message
displays.
6) Click the button to dismiss the message. A confirmation box should
pop up - hitting Cancel should stop the action.
7) Dismiss the message again and this time Confirm. Make sure the
message is gone from the OPAC user summary and from the homepage.
8) Confirm tests pass t/db_dependent/Koha/Patron/Messages.t

Sponsored-by: Koha-US
Comment 56 Christopher Brannon 2023-02-16 18:32:39 UTC
I'm getting this error when I try to load the patch in a sandbox:

TASK [Apply bug 12029 via git-bz in docker container] **************************
fatal: [localhost -> koha-test12029]: FAILED! => {"changed": true, "cmd": "cd /kohadevbox/koha && yes | git bz apply 12029", "delta": "0:00:00.089425", "end": "2023-02-16 18:29:15.316516", "msg": "non-zero return code", "rc": 1, "start": "2023-02-16 18:29:15.227091", "stderr": "Traceback (most recent call last):\n  File \"/usr/bin/git-bz\", line 2716, in <module>\n    applied = do_apply(bug_ref)\n  File \"/usr/bin/git-bz\", line 1738, in do_apply\n    attachmentdata=True)\n  File \"/usr/bin/git-bz\", line 1473, in load\n    bug._load(bug_reference.id, attachmentdata)\n  File \"/usr/bin/git-bz\", line 1229, in _load\n    response = self.server.send_request(\"GET\", url)\n  File \"/usr/bin/git-bz\", line 999, in send_request\n    cookies = self.get_cookie_string()\n  File \"/usr/bin/git-bz\", line 992, in get_cookie_string\n    self.cookies = get_bugzilla_cookies(host)\nNameError: global name 'host' is not defined", "stderr_lines": ["Traceback (most recent call last):", "  File \"/usr/bin/git-bz\", line 2716, in <module>", "    applied = do_apply(bug_ref)", "  File \"/usr/bin/git-bz\", line 1738, in do_apply", "    attachmentdata=True)", "  File \"/usr/bin/git-bz\", line 1473, in load", "    bug._load(bug_reference.id, attachmentdata)", "  File \"/usr/bin/git-bz\", line 1229, in _load", "    response = self.server.send_request(\"GET\", url)", "  File \"/usr/bin/git-bz\", line 999, in send_request", "    cookies = self.get_cookie_string()", "  File \"/usr/bin/git-bz\", line 992, in get_cookie_string", "    self.cookies = get_bugzilla_cookies(host)", "NameError: global name 'host' is not defined"], "stdout": "", "stdout_lines": []}
Comment 57 Owen Leonard 2023-02-16 18:45:58 UTC
The patch applied fine for me in koha-testing-docker so I think this is an issue with the sandboxes
Comment 58 Christopher Brannon 2023-02-16 19:29:30 UTC
Feel free to sign off on it then.  :)
Comment 59 Katrin Fischer 2023-02-17 19:38:14 UTC
It's definitely the issue with the sandboxes we ran into today multiple times. 

Better to have this Needs SO then stuck in FQA.
Comment 60 Christopher Brannon 2023-02-24 21:32:31 UTC
Everything works, however, when I have 3 messages, with two marked read, the counter on the home page of the OPAC still shows 3, even though there is only 1 left to read.  This was addressed in one of the original patches, and is now not working.
Comment 61 Aleisha Amohia 2023-02-28 21:01:03 UTC
Created attachment 147521 [details] [review]
Bug 12029: Ability for patrons to dismiss OPAC messages

This enhancement adds the ability for patrons to dismiss an OPAC
message, marking it as read to remove it from their summary page.

To test:
1) Update database and restart services
2) Log into the staff interface and go to your patron account
3) Click the Add message button
4) Add a message for the OPAC and Save
5) Log into the OPAC. Note there is a message on the homepage saying you
have a message. Go to your user summary and confirm the message
displays.
6) Click the button to dismiss the message. A confirmation box should
pop up - hitting Cancel should stop the action.
7) Dismiss the message again and this time Confirm. Make sure the
message is gone from the OPAC user summary and from the homepage.
8) Confirm tests pass t/db_dependent/Koha/Patron/Messages.t
9) Create a few more messages for the OPAC
10) Log into the OPAC and dismiss one of the messages
11) Confirm the count of unread messages on the OPAC home page is
correct

Sponsored-by: Koha-US
Comment 62 Biblibre Sandboxes 2023-02-28 21:24:28 UTC
Created attachment 147522 [details] [review]
Bug 12029: Adding messages.patron_read_date column

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 63 Biblibre Sandboxes 2023-02-28 21:24:31 UTC
Created attachment 147523 [details] [review]
Bug 12029: Schema updates

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 64 Biblibre Sandboxes 2023-02-28 21:24:34 UTC
Created attachment 147524 [details] [review]
Bug 12029: Tests

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 65 Biblibre Sandboxes 2023-02-28 21:24:37 UTC
Created attachment 147525 [details] [review]
Bug 12029: Ability for patrons to dismiss OPAC messages

This enhancement adds the ability for patrons to dismiss an OPAC
message, marking it as read to remove it from their summary page.

To test:
1) Update database and restart services
2) Log into the staff interface and go to your patron account
3) Click the Add message button
4) Add a message for the OPAC and Save
5) Log into the OPAC. Note there is a message on the homepage saying you
have a message. Go to your user summary and confirm the message
displays.
6) Click the button to dismiss the message. A confirmation box should
pop up - hitting Cancel should stop the action.
7) Dismiss the message again and this time Confirm. Make sure the
message is gone from the OPAC user summary and from the homepage.
8) Confirm tests pass t/db_dependent/Koha/Patron/Messages.t
9) Create a few more messages for the OPAC
10) Log into the OPAC and dismiss one of the messages
11) Confirm the count of unread messages on the OPAC home page is
correct

Sponsored-by: Koha-US
Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 66 Christopher Brannon 2023-02-28 21:25:14 UTC
Works as describe, and totally awesome.
Comment 67 Jonathan Druart 2023-03-23 13:00:52 UTC
Not blocker, but noting:
1. A REST API endpoint would have been better for this job

2. We don't have access to already read notes. Could be added later if needed.
Comment 68 Jonathan Druart 2023-03-23 13:01:53 UTC
Created attachment 148607 [details] [review]
Bug 12029: Adding messages.patron_read_date column

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

JD Amended patch: Fix QA failure "File must have the exec flag"

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 69 Jonathan Druart 2023-03-23 13:01:57 UTC
Created attachment 148608 [details] [review]
Bug 12029: Schema updates

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 70 Jonathan Druart 2023-03-23 13:02:01 UTC
Created attachment 148609 [details] [review]
Bug 12029: Tests

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 71 Jonathan Druart 2023-03-23 13:02:05 UTC
Created attachment 148610 [details] [review]
Bug 12029: Ability for patrons to dismiss OPAC messages

This enhancement adds the ability for patrons to dismiss an OPAC
message, marking it as read to remove it from their summary page.

To test:
1) Update database and restart services
2) Log into the staff interface and go to your patron account
3) Click the Add message button
4) Add a message for the OPAC and Save
5) Log into the OPAC. Note there is a message on the homepage saying you
have a message. Go to your user summary and confirm the message
displays.
6) Click the button to dismiss the message. A confirmation box should
pop up - hitting Cancel should stop the action.
7) Dismiss the message again and this time Confirm. Make sure the
message is gone from the OPAC user summary and from the homepage.
8) Confirm tests pass t/db_dependent/Koha/Patron/Messages.t
9) Create a few more messages for the OPAC
10) Log into the OPAC and dismiss one of the messages
11) Confirm the count of unread messages on the OPAC home page is
correct

Sponsored-by: Koha-US
Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 72 Jonathan Druart 2023-03-23 13:02:08 UTC
Created attachment 148611 [details] [review]
Bug 12029: Remove 'params' from filter_by_unread

It's not used and not needed, you can chain methods.

Also remove unused C4::Koha module and useless statement in tests.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 73 Christopher Brannon 2023-03-23 19:33:53 UTC
(In reply to Jonathan Druart from comment #67)
> Not blocker, but noting:

> 2. We don't have access to already read notes. Could be added later if
> needed.

Staff do, patrons do not.  Yes, something to consider adding later if needed.  Would have to be adjusted on the staff side, as right now, staff have all control as to how long a message sticks around.  We would need to probably have some way of hiding the message when read, like we do on the patron side, and then have a way to access read messages on both sides, and then decide how to manage read messages.  Again, something for another bug perhaps.  This is a good start.
Comment 74 Marcel de Rooy 2023-04-07 07:28:12 UTC
*** Bug 33442 has been marked as a duplicate of this bug. ***
Comment 75 Marcel de Rooy 2023-04-07 07:28:52 UTC
(In reply to Marcel de Rooy from comment #74)
> *** Bug 33442 has been marked as a duplicate of this bug. ***

Nice to see this one :)
Comment 76 Tomás Cohen Arazi 2023-04-20 18:48:16 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 77 Martin Renvoize 2023-05-11 10:30:36 UTC
Nice enhancement, marking as Needs documenting.