Allow different rules and action for delete user #2714

jyeshe · 2024-11-26T11:21:40Z

Description

This PR changes the rendering of the ProfileLive.Edit and UserLive.Index (superuser) liveviews to allow them to be customized on three aspects when deleting a user:

UI rendering
Validation rules
Action processing

Its strategy was similar to the Projects customization, where the liveview is injected on the router.

The change consists on making the respective edit.html.heex and index.html.heex to rely completely on components so that the Saas page can reuse them and implement different rules on the liveview modules.

EDIT: As suggested by the LV docs, the metadata live/4 opt can be used to customize a LiveView which may be invoked from different routes.

Refs #2500

Validation steps

ProfileLive.Edit:

Login with a new user (without project activity)
Navigate User profile and click on Delete account button at the bottom of the page
A popup shows a message: "Your account and credential data ..."
Type the user's email, confirm and the user shall be logged out. On an attempt to login it shows:

UserLive.Index:

Login with a superuser, enter on Admin Menu and click on Users menu
Click on the button to delete the Viewer user
A popup shows a message: "This user's account and credential ..."
Type [email protected], confirm and it shall show the user as scheduled for deletion:
Repeat the operation for the same user and the Viewer user is deleted (doesn't appear anymore on the table).

Additional notes for the reviewer

As a helper note on the refactoring, LightningWeb.ProfileLive.MfaComponent and LightningWeb.ProfileLive.GithubComponent aren't directly related to Delete User and wouldn't need to be extracted from the ProfileLive.FormComponent. However they are not part of the <form> and their actions/events are handled by their own components themselves (as expected) justifying the extraction to the same level as the ProfileLive.FormComponent (and not inside).

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

You can read more details in our Responsible AI Policy

Pre-submission checklist

I have performed a self-review of my code.
I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
I have updated the changelog.
I have ticked a box in "AI usage" in this PR

codecov · 2024-11-26T15:11:15Z

Codecov Report

Attention: Patch coverage is 97.82609% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.25%. Comparing base (ca68969) to head (61c2715).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...ghtning_web/live/components/user_deletion_modal.ex	66.66%	1 Missing ⚠️
lib/lightning_web/live/user_live/index.ex	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2714      +/-   ##
==========================================
+ Coverage   91.22%   91.25%   +0.03%     
==========================================
  Files         333      335       +2     
  Lines       11869    11933      +64     
==========================================
+ Hits        10828    10890      +62     
- Misses       1041     1043       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

elias-ba

Hey @jyeshe, this is really a great piece of work. I learnt a lot while reviewing it, thank you. I left few comments (my reactions and curiosity questions), I would appreciate to hear your thoughts on em.

elias-ba · 2024-11-27T00:45:25Z

lib/lightning_web/live/profile_live/common_components.ex

@@ -0,0 +1,90 @@
+defmodule LightningWeb.ProfileLive.CommonComponents do


@jyeshe CommonComponents is really a great name and I like it. But we usually name these modules as Common or Components, I am wondering if we shouldn't keep that naming for consistency ?

elias-ba · 2024-11-27T00:48:51Z

lib/lightning_web/live/profile_live/common_components.ex

+  def action_cards(assigns) do
+    ~H"""
+    <div id={"user-#{@current_user.id}"} class="md:col-span-2">
+      <.live_component
+        :if={@live_action == :delete}
+        module={@user_deletion_modal}
+        id={@current_user.id}
+        user={@current_user}
+        logout={true}
+        return_to={~p"/profile"}
+      />
+      <.live_component
+        module={LightningWeb.ProfileLive.FormComponent}
+        id={@current_user.id}
+        title={@page_title}
+        action={@live_action}
+        user={@current_user}
+        return_to={~p"/profile"}
+      />
+      <.live_component
+        module={LightningWeb.ProfileLive.MfaComponent}
+        id={"#{@current_user.id}_mfa_section"}
+        user={@current_user}
+      />
+      <.live_component
+        module={LightningWeb.ProfileLive.GithubComponent}
+        id={"#{@current_user.id}_github_section"}
+        user={@current_user}
+      />
+      <.delete_user_card url={@delete_user_url} />
+    </div>
+    """
+  end


elias-ba · 2024-11-27T00:52:05Z

lib/lightning_web/live/user_live/common_components.ex

@@ -0,0 +1,113 @@
+defmodule LightningWeb.UserLive.CommonComponents do


Same question for the naming here ? Again, I really think that CommonComponents is a great name, I am just wondering about module naming consistency.

elias-ba · 2024-11-27T00:54:12Z

lib/lightning_web/live/user_live/common_components.ex

+  attr :users, :list, required: true
+  attr :live_action, :atom, required: true
+
+  attr :user_deletion_modal, :atom,


:atom ? You maybe mean :any (or :map) ?

The atom here is the module name

elias-ba · 2024-11-27T00:55:25Z

lib/lightning_web/live/user_live/index.ex

-       assign(socket, :users, list_users())
-       |> assign(:active_menu_item, :users),
-       layout: {LightningWeb.Layouts, :settings}}
+      modal = Map.get(socket, :private, %{})[:delete_modal]


Nice ❤️

jyeshe · 2024-11-27T16:51:34Z

lib/lightning_web/live/user_live/common_components.ex

@@ -98,7 +98,6 @@ defmodule LightningWeb.UserLive.CommonComponents do
        Cancel deletion
      </.link>
    </span>
-    |


jyeshe · 2024-11-27T19:22:26Z

Changed to use metadata as the private works only on the connection.

…ponents

* There is pre-existing bug that needs to be fixed when clicking outside the modal

stuartc · 2024-12-11T09:48:43Z

lib/lightning_web/live/components/user_deletion_modal.ex

@@ -106,7 +107,7 @@ defmodule LightningWeb.Components.UserDeletionModal do
        </:title>
        <div class="">
          <p class="text-sm text-gray-500">
-            This user cannot be deleted until their auditable activities have also been purged.
+            <%= if @is_superuser_menu, do: "This user", else: "Your account" %> cannot be deleted until their auditable activities have also been purged.


Would it not be better to have the comparison based on the current user vs. the user selected for deletion? Then the component (upstream) wouldn't need to know about its location in the component tree?

I have opted to use the page that uses the modal as a direct and more explicit condition to choose the language:

3rd person on the users page (accessed via superuser menu)

1st person on the user profile

Maybe I should have used the page name instead of the menu as a context.

The condition you propose is valid now that we've blocked the deletion of super users and also referring to the superuser on the 3rd person, when deleting his/her own account wouldn't harm much as well. The main reason was to make the condition as clearer as possible (kind of giving names from user story/business perspective).

But the 3rd and 1st person perspective can be determined by the current user and the selected user, I'm not seeing a good enough reason to bring the layout into the mix when we have the current user available everywhere.

If the @is_superuser_menu option is purely for the copy, I would suggest an attr that controls it lives outside, like is_current_user or something. Components really should avoid knowing about the place they are mounted from.

I get your point of sufficient existing conditions but it is not rare that find components that behave differently according to a context.
On the other hand, is_current_user is very straightforward and clear as well. Thanks

stuartc · 2024-12-11T10:00:22Z

lib/lightning_web/live/user_live/index.ex

  end

  defp apply_action(socket, :delete, %{"id" => id}) do
    socket
    |> assign(:page_title, "Users")
-    |> assign(:user, Accounts.get_user!(id))
+    |> assign(:delete_user, Accounts.get_user!(id))


Why the change? It's called user downstream in the table component as well. I could imagine it's because we're wanting to signify that this user is selected for deletion, but the component doesn't really know about that, it's just a 'selected user' in my mind and is only deletable when paired with an action/event.

The renaming was for three reasons:

It's not used anymore directly neither on the LV nor on the heex, it's passed down to 3rd level component.

Same idea as you've mentioned the selected user that will be used somewhere for some operation

Given the 2 above, I wanted to make it easier for the review and maintenance, in a way that it's easy to search and quickly find it by a unique name:

jyeshe force-pushed the polimorphic-delete-user branch 4 times, most recently from 4e32c32 to 7d06f11 Compare November 26, 2024 13:01

jyeshe changed the title ~~Allow polimorphic rules and action for delete user~~ Allow different rules and action for delete user Nov 26, 2024

jyeshe force-pushed the polimorphic-delete-user branch 4 times, most recently from 1418886 to 79a937d Compare November 26, 2024 13:13

jyeshe self-assigned this Nov 26, 2024

jyeshe linked an issue Nov 26, 2024 that may be closed by this pull request

Scheduled deletion of users account does not work #2500

Closed

jyeshe force-pushed the polimorphic-delete-user branch from 79a937d to ed7793f Compare November 26, 2024 14:31

jyeshe marked this pull request as ready for review November 26, 2024 14:31

jyeshe requested review from stuartc, midigofrank, elias-ba and taylordowns2000 and removed request for midigofrank November 26, 2024 14:31

jyeshe force-pushed the polimorphic-delete-user branch 2 times, most recently from 02ce262 to 4dfd455 Compare November 26, 2024 14:54

elias-ba approved these changes Nov 27, 2024

View reviewed changes

jyeshe commented Nov 27, 2024

View reviewed changes

jyeshe force-pushed the polimorphic-delete-user branch 2 times, most recently from 20ebc32 to 6528aac Compare November 27, 2024 18:12

jyeshe force-pushed the polimorphic-delete-user branch 4 times, most recently from 17f55c0 to a93a560 Compare November 28, 2024 11:04

jyeshe force-pushed the polimorphic-delete-user branch 5 times, most recently from 918236f to 5159368 Compare November 28, 2024 20:23

jyeshe requested a review from midigofrank November 29, 2024 09:35

jyeshe and others added 8 commits December 10, 2024 13:12

Refactor user profile for remaining components

481ceb2

Rename components module

2cc195e

Remove unnecessary vertical bar between table actions in UserLive.Com…

fda2e31

…ponents

Replace not working private with metadata

ee8625b

Address the user directly yet once more

b558147

Fix use of metadata modal

b92fa84

Fix cancel scheduled deletion on admin users

62a3d6d

Revert change

483c769

* There is pre-existing bug that needs to be fixed when clicking outside the modal

jyeshe force-pushed the polimorphic-delete-user branch 10 times, most recently from 3380fa4 to fc4aba7 Compare December 10, 2024 16:00

Fix test cases and revert to attrs string keys

a95852f

jyeshe force-pushed the polimorphic-delete-user branch from fc4aba7 to a95852f Compare December 10, 2024 16:30

stuartc approved these changes Dec 11, 2024

View reviewed changes

Rename menu context to current user

61c2715

stuartc merged commit c9cad42 into main Dec 11, 2024
8 checks passed

stuartc deleted the polimorphic-delete-user branch December 11, 2024 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow different rules and action for delete user #2714

Allow different rules and action for delete user #2714

jyeshe commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

elias-ba left a comment

elias-ba Nov 27, 2024

jyeshe Nov 27, 2024

elias-ba Nov 27, 2024

elias-ba Nov 27, 2024

jyeshe Nov 27, 2024

elias-ba Nov 27, 2024

jyeshe Nov 27, 2024 •

edited

Loading

elias-ba Nov 27, 2024

jyeshe Nov 27, 2024

jyeshe commented Nov 27, 2024

stuartc Dec 11, 2024

jyeshe Dec 11, 2024

jyeshe Dec 11, 2024 •

edited

Loading

stuartc Dec 11, 2024

jyeshe Dec 11, 2024

stuartc Dec 11, 2024

jyeshe Dec 11, 2024 •

edited

Loading

		@@ -0,0 +1,90 @@
		defmodule LightningWeb.ProfileLive.CommonComponents do

		@@ -0,0 +1,113 @@
		defmodule LightningWeb.UserLive.CommonComponents do

Allow different rules and action for delete user #2714

Allow different rules and action for delete user #2714

Conversation

jyeshe commented Nov 26, 2024 • edited Loading

Description

Validation steps

Additional notes for the reviewer

AI Usage

Pre-submission checklist

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

elias-ba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyeshe Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyeshe commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyeshe Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyeshe Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

jyeshe commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

jyeshe Nov 27, 2024 •

edited

Loading

jyeshe Dec 11, 2024 •

edited

Loading

jyeshe Dec 11, 2024 •

edited

Loading