Add ability to cancel "read all"

[clarkwinkelmann]

Fixes flarum/issue-archive#341

Changes proposed in this pull request:
Remove confirmation and add ability to cancel "read all" within 10 seconds.

This PR was made during my "clark writes code" stream, I will check it back later for completion.

Reviewers should focus on:

Screenshot

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

Required changes:

• Related core extension PRs: (Remove if irrelevant)

[dsevillamartin]

Do we want tests for this feature?

[clarkwinkelmann]

I'm not super satisfied with the approach on this, I don't feel like the way I did the backend goes well with the existing Flarum codebase.

Another option would be to wait in the frontend for the "cancel" timeout, and only send the "read all" request if the user didn't cancel. That wouldn't require any change to the backend. But that would mean that if you click "read all", and immediately close the tab, it will actually never save the "read all" state.

[dsevillamartin]

@clarkwinkelmann Hm, that's another idea yeah. I think there's ways of listening to the tab close and doing something before it closes (e.g. do you really want to leave popups) with beforeunload window event (sometimes browsers will ignore prompts, confirms, alerts... here, but a JSX request may work?).

Honestly I don't think the request not sending would be that big of a deal. You could have a timeout with the message, "marking as read in 5 seconds cancel", if we do want to do that.

Let's wait for other input on this matter though.

[clarkwinkelmann]

Honestly I don't think the request not sending would be that big of a deal. You could have a timeout with the message, "marking as read in 5 seconds cancel", if we do want to do that.

I actually like that version better. But yeah let's hear from someone else.

Using the session in Flarum is very troublesome. Ideally the mark all feature should go to a command, but the command wouldn't have a way to access the session unless the session is passed to the command from the controller.

[luceos]

What's the benefit of this over the confirmation dialog?

[clarkwinkelmann]

One less click when you don't need to cancel 🤷‍♂️ That was based on the issue linked above.

By the way I noticed Gmail does block page unload if there's the cancel button in the bottom, suggesting they might do the request a bit after the message shows.

So I think if we do implement this, it's probably better to just wait until the warning expires to actually submit. This way there's no change required in the backend. I'd say we close this PR and have another one made.

[nxta]

Hi. I'm new here. I enjoyed the stream!

Can I please make a proposal for this type of UX flow. You're going to encounter lots more actions that you wish could be undone throughout the application.

You already have an AlertManager to notify when a certain action is done. If Alerts could contain controls, this is where I would place the undo button.

I actually JUST noticed that this is how YouTube does it

But yeah let's hear from someone else.

I don't see a reason to put a time limit on undoing anything. In fact I only see problems with that

[clarkwinkelmann]

@nxta thanks for the feedback. I had considered that option when writing the PR, and wasn't sure whether it was appropriate. Right now I think alerts always stay forever, so we would require some kind of timeouts for alerts. I would definitely be in favor of that option if we implement the few additions for the alert manager.

If we make it look similar to Gmail/YouTube, most people should be familiar with the system.

It's true that we could make other actions cancellable. So in this case it would make sense to have something reusable.

The reasoning behind the placement here is that I wanted to keep the cancel button close to the "mark all read" button. But as I said above I'm not very satisfied so will probably rewrite this anyway.

I don't see a reason to put a time limit on undoing anything. In fact I only see problems with that

I'm not sure I understand which system you prefer. The question was whether we do the action immediately, and allow cancelling through the backend (that's the solution implemented in this PR), or if we simply delay the API call and allow to cancel before that call is made instead.

The latter solution has the advantage of requiring only frontend changes, can not introduce integrity or security issues in the backend, and could easily be implemented for any feature in Flarum without complex changes. The only drawback is that the actions are then not immediate, as they won't happen until the delay is reached, and require a bit of code to make sure you can't accidentally close the tab and cancel the action without noticing.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[askvortsov1]

Are we tweaking this or starting from scratch @clarkwinkelmann?

[clarkwinkelmann]

I didn't notice this was still open.

I personally prefer the "Google" solution, because it could be re-used for other parts of Flarum and extensions, and doesn't introduce any issue of rolling back information in the database.

Closing this.

[clarkwinkelmann]

Amendment to last post: I forgot there were two discussions.

Regarding the UI: I prefer the "Google" approach (alert at the bottom of the screen with UNDO button). We can re-use the alert feature, and could make a custom alert that's re-usable elsewhere.

Regarding the logic, I prefer we delay the action in the frontend, and only send the request after the timeout passed. And we need to prevent closing the page during that time. This way no change is needed in the backend, and it's also easier to re-use the same logic elsewhere.

[askvortsov1]

Looks like we're on the same page then (https://github.com/flarum/core/issues/878#issuecomment-594306999)! I'll try and work on this.