mgr/dashboard: Refactoring of DeletionModalComponent

[p-se]

• Simpler variable names:

  Examples:

  • actionDescription instead of metaType
  • bodyTemplate instead of description
  • validationPattern instead of pattern

  Some of these variable names have been generalized to ease the
  unification/generalization of dialog components:

  • submitAction instead of deletionMethod

• Removed unique setUp method.

  Benefits:

  • Creation of the component is done as intended by the developers of
    the ngx-boostrap package and as expected by developers which use
    the package. The setUp method does not have to be called anymore
    on the DeletionModalComponent exclusively but instead the
    component is instantiated as all other modals. Property assignment
    on the instantiated object isn't handled by the setUp method
    anymore but by the modalService.

  • With the removal of the setUp method, some tests could be
    removed as well.

  • No need to pass the reference of the created modal to the modal
    manually.

  Preserved:

  • The provided check within the setUp method, which checked if the
    component had been correctly instantiated, has been moved to the
    ngOnInit method of the component.

Signed-off-by: Patrick Nawracay pnawracay@suse.com

[p-se]

jenkins retest this please

[Devp00l]

You have to prettify your code to get all tests to pass.
Besides that good job 👍

[Devp00l]

The actionDescription is missing here which results in a broken modal and that the following error is thrown in console No action description defined.

[p-se]

Nice find

[Devp00l]

Nice refactoring 👍

[p-se]

Due to recently merged changes I had to adapt the code. We'll now have two variables for describing what is being confirmed: One variable describes the action (actionDescription), that will be "delete" in most but not all cases and one describes the item (itemDescription) (User, OSD, RBD, RBD snapshot, etc). Using those variables to craft the text of the modal, it's not possible to say "I'm sure I want to proceed with the deletion" anymore. "Deletion" doesn't work anymore because only the value "delete" is passed to the modal (to correctly craft the button and title text). Hence I adapted the text to be more concise and fit more use cases:

I'll be using this new capability of the DeletionModalComponent (it's actually a CriticalConfirmationModalComponent by now) to implement OSD destroy and OSD remove. Later it will also be possible to purge an OSD. In all those cases "delete" will be replaced by their corresponding actions (destroy, remove, purge). I decided to do so to keep the original descriptions of Ceph operations in the dashboard (as we're used to) and to make the component usable for all kind of critical operations where we want the user to confirm that he/she knows what he/she is doing.

The questions that came up are:

• Are you fine with the adaption of the text of the checkbox?
• Does it make sense to rename the component to CriticalConfirmationModalComponent? I'm also fine with DeletionModalComponent although it won't exclusively be used for delete operations but likely in most cases.

PR has been updated, please review again.

[votdev]

What about introducing a customizable ConfirmationModalComponent and derive the DeletionModalComponent from that. Thus we have a component that can be customized by the developer to its needs and we have a standardized deletion dialog.

[p-se]

@votdev We already have a ConfirmationModalComponent, no need to introduce that one. The effort to refactor the DeletionModalComponent, despite the need to adapt the text "delete" to anything else, was due to being able to refactor all components into generalized ones, which then are supposed to be used by the developers to have standardized deletion dialogs.
So yes, I think bringing both together makes sense, but it's out of scope for this PR as that one was just needed to enable implementing OSD Actions to the frontend (which I planed to do first).
But I wouldn't be opposed to bringing the ConfirmationDialog together with the (let me call it) CriticalConfirmationDialog, but the way to do that doesn't have to be inheritance or derivation. It might make sense to have one component that does both, enable the developer to have non-critical confirmations and critical confirmations. That would need to be determined.
By the way, I prefer "Confirmation" over "Deletion" as it's more generalized and is likely to offer more of the required functionality (like described above -> destroy, purge, remove, etc).

[Devp00l]

@p-na IMHO it's fine to rename it if it will be used differently in a not so distant future. The rest looks good to me 👍

[votdev]

Don't use the titlecase pipe for itemDescription, otherwise RBD snapshot will be transformed to Rbd Snapshot.

[tspmelo]

lgtm

[ricardoasmarques]

LGTM

Addressed