FEATURE/DEV: Ask user to confirm topic deletion when views exceed a certain amount (PR #10459)

Before deleting a topic that has a high number of views (default of 5000), the user will be prompted with a confirmation popup. This works for all delete buttons on the topic located in: topic-timeline, topic-admin-menu, topic-footer-buttons, and post-menu if the post’s ID is 1.

The delete button will be disabled while deletion is in progress, to prevent any unwanted behavior.

A site setting is also available to change the minimum amount of views required to display the confirmation popup.

GitHub

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

:white_check_mark: RickyC0626
:x: AndreaHabib
You have signed the CLA already but the status is still pending? Let us recheck it.

There’s no HTML in this key so I don’t think it should be passed through the html-safe helper.

This is looking good so far and I think it’s a nice feature. However I think you will also need to add a qunit test to make sure it’s working.

I suggest using the not computed property macro here. It’s shorter.

You probably don’t need an alias here. In the template you can use the full key path and it’ll work.

Trivial but you can combine these two imports into one.

          this.set('deleting', false);

setProperties should only be used when updating multiple properties.

I’m a bit confused as to how the not computed property can be used here, can you please elaborate?

My understanding is that you are trying to convert buttonDisabled to a boolean. Why do you need it to be a boolean and a truthy / falsey value would not suffice?

I think it would be a bit better if you reset this property in onShow(). We try to reset the controllers before reusing them to get rid of any undesired state.

Check how it is used in other modal controllers.

Great work! Feel free to ignore my comments as they are more of a nit-pick. :+1:

I agree with @eviltrout that you will have to add a QUnit test.

If you implement the suggestion above, you can remove this line of code (by the time you hit finally, the modal would have been closed in the then handler.

Oh, but you might have to set here this.set("deleting", false) to let the user try to delete one more time (in case it is an intermittent issue)?

Ahh, I think I understand what you mean now. I have seen it being used in other components.

I ended up deleting the function completely and just accessed the variable directly from the template. onShow initializes the default value anyway. The only other function that accesses the deleting variable (now changed to isTopicDeleting) is buttonTitle, which is just a minor UX/UI feature to let the user know that the topic is in the process of being deleted, rather than just a disabled button.

can you use @action please ? you will see examples at other places in the codebase

Please use { } we don’t use this kind of syntax elsewhere in the codebase

would be better with a deletingTopic declaration, eg: deletingTopic: false,

Will do, thank you