Add status param support for dismissing all notes #7743
Conversation
So, would we still see this issue if there were more than 25 |
mattsherman
left a comment
mattsherman
left a comment
There was a problem hiding this comment.
Tested, works as expected. Nice work.
Left a comment about unit tests -- I think it would be good to have a unit test or two in place to test this new functionality.
I also left a general question about whether this issue would will crop up if there were actually more than 25 unactioned notes. I think it is okay for now to live with that limitation if that is the case. However, we should add a new issue in the backlog about that, so we don't forget to cycle back and full address it later.
| 'methods' => \WP_REST_Server::DELETABLE, | ||
| 'callback' => array( $this, 'delete_all_items' ), | ||
| 'permission_callback' => array( $this, 'update_items_permissions_check' ), | ||
| 'args' => array( |
There was a problem hiding this comment.
It would be good to add unit test(s) around this new capability.
woocommerce-admin/tests/api/admin-notes.php
Line 483 in 2c02a45
There was a problem hiding this comment.
@mattsherman good suggestion, I added a test case here: 39a4647
mattsherman
added
status: need author's reply
release: cherry-pick
and removed
status: needs review
release: cherry-pick
labels
I think it will delete the first 25, but the next deletion would get the subsequent 25 since it filters by Not sure if this is better or genuinely deleting all. I can see an argument being made for either case. |
Yeah, I too think this might actually be desirable behavior. Any time the store merchant is dealing with more than 25 inbox notes, I would be concerned that they might mistakenly delete ones they didn't notice. Of course, the UI then "lies" by saying "Delete All", but is it really that harmful of a lie? :-) If a merchant really wanted to get rid of any of the remaining notes, they would likely try "Delete All" again before contacting support. |
Yeah I was thinking something similar, given we only display 25 max on the client, I would take 'Delete all' to mean, delete all the one's visible, not the one's invisible. Although this would almost require some kind of page identifier, we are displaying 25, but there are more. |
| wp_set_current_user( $this->user ); | ||
|
|
||
| // It deletes only unactioned notes. | ||
| $request = new WP_REST_Request( 'DELETE', $this->endpoint . '/delete/all' ); |
There was a problem hiding this comment.
Just a side comment... that's pretty bizarre that our API uses both the DELETE HTTP verb and a URL that includes /delete... such a mismash.
mattsherman
left a comment
mattsherman
left a comment
There was a problem hiding this comment.
Thanks for adding the unit test! I had trouble with my unit testing setup locally, but the test code looks valid so I think this is good to go!
* Add status param support for dismissing all notes * Add changelog * Add unit test for new arg in /delete/all endpoint
…merce-admin#7743) * Add status param support for dismissing all notes * Add changelog * Add unit test for new arg in /delete/all endpoint
Fixes #7741
Add status query param to
delete_all_notesso query is run correctly.The underlying issue was that the
delete/allquery queried for any status, causing the notes to be returned to be greater than 25 (the page limit). Although on our client we only displayunactionednotes, I added support for this query arg.note
I only added support for status to fix this particular issue. We could potentially support any query arg in the same way we support this for the
notes/allendpoint.Screenshots
Detailed test instructions: