Bug 12029

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.

(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.

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.

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.

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.

(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.

(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.

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.

(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.

(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?

> 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.

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.

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.

(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.

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.

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>

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.

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!

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 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?

(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!

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>

Thank you for the follow-up Hayley!

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!

Auto-Parallel, who wrote the patch, is supposed to be uploading the unit tests.  We are waiting for them to do this.

Thx for the update, Christopher. 

If we could get the unit tests within the next few days, we still might make the release.

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.

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"

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

Created attachment 137520 [details] [review]
Bug 12029: DBIC Schema

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'

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>

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>

Created attachment 137523 [details] [review]
Bug 12029: DBIC Schema

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

All appears to work as designed.. signing off, ready for QA :)

(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?

(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.

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>

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>

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>

Looks good to me as well.

DB naming (suffix _date), see bug 31150.

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.

+    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.

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.

Created attachment 146706 [details] [review]
Bug 12029: Adding messages.patron_read_date column

Created attachment 146707 [details] [review]
Bug 12029: Schema updates

Created attachment 146708 [details] [review]
Bug 12029: Tests

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

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": []}

The patch applied fine for me in koha-testing-docker so I think this is an issue with the sandboxes

Feel free to sign off on it then.  :)

It's definitely the issue with the sandboxes we ran into today multiple times. 

Better to have this Needs SO then stuck in FQA.

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.

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

Created attachment 147522 [details] [review]
Bug 12029: Adding messages.patron_read_date column

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

Created attachment 147523 [details] [review]
Bug 12029: Schema updates

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

Created attachment 147524 [details] [review]
Bug 12029: Tests

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

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>

Works as describe, and totally awesome.

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.

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>

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>

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>

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>

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>

(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.

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

(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 :)

Pushed to master for 23.05.

Nice work everyone, thanks!

Nice enhancement, marking as Needs documenting.