Skip to content

feat: Account delegation#12727

Open
hamza221 wants to merge 3 commits intomainfrom
feat/delegation-mvp
Open

feat: Account delegation#12727
hamza221 wants to merge 3 commits intomainfrom
feat/delegation-mvp

Conversation

@hamza221
Copy link
Copy Markdown
Contributor

@hamza221 hamza221 commented Apr 14, 2026

Mvp for #12403

1 2 3 4
image image image image

Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
@hamza221 hamza221 changed the title feat: add delegation backend feat: Account delegation Apr 14, 2026
@hamza221 hamza221 added enhancement 3. to review skill:backend Issues and PRs that require backend development skills skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills labels Apr 14, 2026
@hamza221 hamza221 self-assigned this Apr 14, 2026
@hamza221 hamza221 requested a review from nimishavijay April 14, 2026 15:05
@hamza221 hamza221 marked this pull request as ready for review April 14, 2026 15:30
@hamza221 hamza221 requested a review from kra-mo April 14, 2026 15:32
@hamza221
Copy link
Copy Markdown
Contributor Author

Copied over design review from the other pr

Nimisha

Awesome! Looks really nice! Only some small changes. Overall:

  • The icon for delegation (across the board) can be supervisor account. The one you've chosen is also very similar but we use that for indicating users and groups
  • Instead of "Delegation", I'm wondering if we can use the wording "Delegate account" in the 3 dot menu and the modal title
  • Use tertiary buttons for all the "Cancel" buttons and don't use the icon.

In the "Delegation" modal:

  • There is some whitespace on top which shouldn't be there (somewhow not there in the "Add delegates" modal)
  • We can also center align the "Delegates" heading. Ik in kra-mo's mockups it was left aligned, but because in the component we always have the heading in the center it looks inconsistent now.
  • I see that there's a h3 { font-weight: bold; } which makes the "Delegates" heading stronger than its parent heading. We could remove that

In the "Add delegates" modal:

  • the select component should be full width

Laura

We can also center align the "Delegates" heading.

I was going off of form group components, not a dialog, in this context, I think the two different headings don't make sense either, we could just have one "Delegates" title :)

Laura

Adding to what nimishavijay said, the "Revoke" button should also probably not have an icon, a checkmark with danger styling is always confusing :)

Additionally, the user selection dropdown should be full-width and there should be a bit more padding between the explanation text beneath and the buttons, also in the revoke access dialog.

}

/**
* @NoAdminRequired
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use #[NoAdminRequired] attribute

Comment on lines +42 to +43
* @param int $accountId
* @return JSONResponse
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param int $accountId
* @return JSONResponse

Please remove redundant (=> dont provide any additional knoweldge to the type hints) phpdoc

public function delegate(int $accountId, string $userId): JSONResponse {

$account = $this->accountService->findById($accountId);
if ($this->currentUserId === null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flip with the line above

#[TrapError]
public function delegate(int $accountId, string $userId): JSONResponse {

$account = $this->accountService->findById($accountId);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would recommend find with try+catch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review enhancement skill:backend Issues and PRs that require backend development skills skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants