CPT / State: Display trash notice after save request completes successfully

[aduth]

This pull request seeks to explore one option for displaying a notice in response to a successful network request to save a post. It also seeks to refactor notices state to better accommodate this use-case and to adhere to guidelines adopted since its original implementation (e.g. action type verb suffixing).

Implementation notes:

I considered a few options for implementing notices:

• Dispatch successNotice in the promise resolution of savePost
  • Pros:
    • Colocation to related actions leading to notice creation
    • Notices shown even if navigating away from screen
  • Cons:
    • Difficult to differentiate user interaction from background process where notice may be undesirable (maybe options argument?)
    • Doesn't seem possible to track notice state like count (i.e. "2 posts trashed")
• Track success and failure in a separate reducer specific to posts, which components monitor and dispatch notice when componentWillReceiveProps reveals that a success or failure has occurred
  • Pros:
    • Feels most pure in terms of one-way-data-flow
    • If "notices shown after navigating away" is argued to be a bad thing, then this is most obvious path to achieving the opposite effect
  • Cons:
    • Notice won't be shown if user navigates away (or component is otherwise unmounted)
    • Seems difficult to implement correctly, and likely more code to do so than other solutions
• [As implemented here, ] Monitor actions from the notices reducer
  • Pros:
    • If considering action types as global, appropriate for notices reducer to "watch" for actions which should cause a notice to be shown
    • All behaviors related to notices mostly within a single file
    • Notices shown even if navigating away from screen
  • Cons:
    • Splits action creator invoking the save from notices behavior (maybe a pro?)
    • Should notices reducer concern itself with actions from other parts of state? (seems an argument against actions as being global)

Testing instructions:

Ensure Mocha tests pass:

npm run test-client

Verify that no regressions occurred in refactorings to expected behavior for global notices.

Verify that on the custom post types listing screen, when trashing a post, a notice is shown if it completes successfully. Verify also that if trashing a second post while a notice is currently visible, the text will change to reflect "X posts" trashed.

/cc @artpi @gwwar @timmyc

Test live: https://calypso.live/?branch=add/cpt-trash-notice

[artpi]

@aduth, you may want to check @taggon 's PR - he did lots of amazing work with notices and I think there can be some overlap: #4821
@nb is merging this soon.

[aduth]

Thanks for the pointer @artpi , I hadn't seen that pull request. Looks like some good effort going on there, though not sure it necessarily solves the problem I'm trying to address (notices in response to asynchronous network requests). I do see that it touches on notices behavior in Reader in response to updated props, which is a bit like option 2 I had considered in the original comment.

[gwwar]

👍 right, omit handles this case, but it would return a new object.

[aduth]

👍 right, omit handles this case, but it would return a new object.

Indeed, though probably very unlikely NOTICE_REMOVE would ever be called with an invalid noticeId.

[gwwar]

Took the branch for a spin and wasn't able to find any obvious regressions.

👍 I'd be fine with going forward with this approach. If we do, we should update documentation on how to add a notice, and deprecate the createNotice action.

[aduth]

If we do, we should update documentation on how to add a notice, and deprecate the createNotice action.

Interesting, I hadn't thought too much on the implications these changes would have on deprecating the action creators altogether. Now that you bring it up, I suppose it would be good to have the consistency, and can't really think of a case where we'd want to explicitly call the action creator instead of hooking in via the reducer (might be a bit of a pain to create action types for e.g. DevDocs example).

[gwwar]

might be a bit of a pain to create action types for e.g. DevDocs example

To keep the number of test actions to a minimum, I think it's fine to use the createNotice action for these test cases, but maybe leave some pretty clear notes in the code to show what our expected pattern is.