Skip to content

Conversation

@0x46616c6b
Copy link
Member

Added a new tile for domain admins and a corresponding view to add two essential actions: add new accounts and add new aliases.

Bildschirmfoto am 2024-09-28 um 19 21 54

Bildschirmfoto am 2024-09-28 um 19 21 57

@0x46616c6b 0x46616c6b added the enhancement New feature or request label Sep 28, 2024
@0x46616c6b 0x46616c6b force-pushed the Add-Domain-Settings branch 6 times, most recently from e7c9755 to 22372d6 Compare September 28, 2024 19:37
@0x46616c6b 0x46616c6b marked this pull request as ready for review September 28, 2024 19:38
@sonarqubecloud
Copy link

@0x46616c6b 0x46616c6b requested a review from y3n4 as a code owner April 10, 2025 21:00
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
12.4% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Copy link
Contributor

@doobry-systemli doobry-systemli left a comment

Choose a reason for hiding this comment

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

Nice work! I stumbled upon some problems though that need to be addressed.

And I should see text matching "Return to Index"

@admin
Scenario: Access to Admin Interface as Domain Admin
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea to keep these tests, but would rename the scenario:

No access to Admin Interface as Domain Admin


domain_settings:
title: Domain settings
intro: Here you can manage your domain settings.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better subtitle: "Manage accounts and aliases for domain example.org"?

$this->addFlash('error', 'domain_settings.form-error');

return $this->render('DomainSettings/index.html.twig', [
'registration' => $form->createView(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Here the alias form view needs to be passed as well, otherwise the template throws an error because alias is unset.

$this->addFlash('error', 'domain_settings.form-error');

return $this->render('DomainSettings/index.html.twig', [
'alias' => $form->createView(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Here the registration form view needs to be passed as well, otherwise the template throws an error because registration is unset.

$this->addFlash('error', 'domain_settings.form-error');
}

return $this->redirectToRoute('domain_settings');
Copy link
Contributor

Choose a reason for hiding this comment

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

After a user got successfully added, we should show a (non-volatile) notification that the new user should log in and create a recovery token. Or we should create the recovery token straight away here and display it afterwards.

$this->registrationHandler->handle($data->getEmail(), $data->getPlainPassword());
$this->addFlash('success', 'domain_settings.registration-success');
} catch (\Exception) {
$this->addFlash('error', 'domain_settings.form-error');
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason I always get this error message, even if creating the user succeeded.

use PlainPasswordTrait;

#[Assert\Email(message: 'form.invalid-email', mode: 'strict')]
#[EmailAddress]
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like these two assertions both result in very similar error messages:

The e-mail is not valid.
The email is invalid.

image

Either we should harmonize them and only show one if both are same, or make the more verbose in case that's possible.

{{ form_start(registration) }}
{{ form_errors(registration) }}
<div class="form-group">
{{ form_label(registration.email) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

"Your preferred e-mail address" is a bit misleading here as label. Maybe just make it "E-mail address"? 🤔

{{ form_widget(registration.plainPassword.second, {'attr': {'class': 'form-control input-md' }}) }}
</div>
<div class="form-group">
{{ form_widget(registration.submit, {'attr': {'class': 'btn btn-md btn-primary btn-block' }}) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here the label is "submit", while it's "Add alias address" for the alias form. Should be harmonized and become "Add e-mail address" here.

{{ form_errors(alias) }}
<div class="form-group">
{{ form_label(alias.alias) }}
{{ form_errors(alias.alias) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason the errors from the alias form get displayed in the toast message instead of being displayed above the corresponding input field. For the address from it works as expected.

@0x46616c6b 0x46616c6b closed this Aug 19, 2025
@0x46616c6b 0x46616c6b deleted the Add-Domain-Settings branch August 29, 2025 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants