Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show registration errors #3588

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions app/config/config_dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ monolog:
level: info
max_files: 3
channels: [dabba]
register:
type: stream
path: "php://stdout"
level: debug
include_stacktraces: true
channels: [register]
# channels: [register]
# uncomment to get logging in your browser
# you may have to allow bigger header sizes in your Web server configuration
#firephp:
Expand Down
10 changes: 9 additions & 1 deletion app/config/services.yml
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ services:
League\Flysystem\Filesystem $taskImportsFilesystem: '@task_imports_filesystem'
League\Flysystem\Filesystem $receiptsFilesystem: '@receipts_filesystem'
Nucleos\UserBundle\Util\CanonicalizerInterface: '@nucleos_user.util.canonicalizer.default'

Spatie\GuzzleRateLimiterMiddleware\Store: '@AppBundle\Geocoder\RateLimiterStore'

# Autowiring variables in controllers
Expand Down Expand Up @@ -212,6 +212,8 @@ services:
coopcycle.user_provider:
class: AppBundle\Security\NucleosUserBundleUserProvider
arguments: ['@nucleos_user.user_manager', { facebook: facebookId }]
tags:
- { name: monolog.logger, channel: register }

AppBundle\EventListener\MaintenanceListener:
arguments:
Expand Down Expand Up @@ -581,6 +583,8 @@ services:
- '@coopcycle.user_manager.inner'
- '@nucleos_user.object_manager'
class: AppBundle\Security\UserManager
tags:
- { name: monolog.logger, channel: register }

AppBundle\Service\SettingsManager:
arguments:
Expand Down Expand Up @@ -697,6 +701,8 @@ services:
arguments:
$confirmationEnabled: '%nucleos_profile.registration.confirmation.enabled%'
$mailer: '@nucleos_profile.mailer.default'
tags:
- { name: monolog.logger, channel: register }

AppBundle\Action\ResettingSendEmail:
public: true
Expand Down Expand Up @@ -1025,6 +1031,8 @@ services:
$orderRepository: '@sylius.repository.order'
$customerRepository: '@sylius.repository.customer'
$secret: '%secret%'
tags:
- { name: monolog.logger, channel: register }

AppBundle\Embed\Context: ~

Expand Down
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ services:
/usr/bin/mc mb --region $S3_REGION --ignore-existing coopcycle/images/restaurants/;
/usr/bin/mc mb --region $S3_REGION --ignore-existing coopcycle/images/stores/;
/usr/bin/mc mb --region $S3_REGION --ignore-existing coopcycle/images/tasks/;
/usr/bin/mc policy set public coopcycle/images/;
/usr/bin/mc anonymous set public coopcycle/images/;
"

tile38:
Expand Down
52 changes: 48 additions & 4 deletions src/Action/Register.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,23 @@
use Symfony\Component\Validator\ConstraintViolationInterface;
use Symfony\Component\Validator\ConstraintViolationList;
use Symfony\Component\Validator\Validator\ValidatorInterface;
use Psr\Log\LoggerInterface;
use Psr\Log\NullLogger;
use Monolog\Logger;
use Monolog\Handler\StreamHandler;

use Nucleos\ProfileBundle\Event\GetResponseRegistrationEvent;
use Nucleos\ProfileBundle\Event\UserFormEvent;
use Nucleos\ProfileBundle\Form\Model\Registration;
use Nucleos\ProfileBundle\Form\Type\RegistrationFormType;
use Nucleos\ProfileBundle\NucleosProfileEvents;
use Nucleos\UserBundle\Event\FilterUserResponseEvent;
use Nucleos\UserBundle\Event\FormEvent;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\RouterInterface;
use Twig\Environment;

class Register
{
Expand All @@ -31,6 +48,10 @@ class Register
private $tokenGenerator;
private $mailer;
private $confirmationEnabled;
/**
* @var Environment
*/
private $twig;

public function __construct(
UserManagerInterface $userManager,
Expand All @@ -40,7 +61,10 @@ public function __construct(
TokenGeneratorInterface $tokenGenerator,
MailerInterface $mailer,
ValidatorInterface $validator,
bool $confirmationEnabled)
bool $confirmationEnabled,
LoggerInterface $registerLogger,
Environment $twig,
)
{
$this->userManager = $userManager;
$this->jwtManager = $jwtManager;
Expand All @@ -50,6 +74,8 @@ public function __construct(
$this->mailer = $mailer;
$this->validator = $validator;
$this->confirmationEnabled = $confirmationEnabled;
$this->registerLogger = $registerLogger;
$this->twig = $twig;
}

/**
Expand All @@ -61,14 +87,31 @@ public function __construct(
*/
public function registerAction(Request $request)
{
// create a log channel
$log = new Logger('nameregisterAction');
$log->pushHandler(new StreamHandler('php://stdout', Logger::WARNING)); // <<< uses a stream

// add records to the log
$log->warning('Foo registerAction');
$log->error('Bar registerAction');
$email = $request->request->get('_email');
$username = $request->request->get('_username');
$password = $request->request->get('_password');
$password_confirmation = $request->request->get('_password_confirmation');
$telephone = $request->request->get('_telephone');
$givenName = $request->request->get('_givenName');
$familyName = $request->request->get('_familyName');
$fullName = $request->request->get('_fullName');

$this->registerLogger->info('Request data : email={email}; username={username}; password={password}; password_confirmation={password_confirmation}; telephone={telephone}; givenName={givenName}; familyName={familyName}; fullName={fullName};', [
'email' => $email,
'username' => $username,
'password' => $password,
'password_confirmation' => $password_confirmation,
'telephone' => $telephone,
'givenName' => $givenName,
'familyName' => $familyName,
'fullName' => $fullName
]);
$data = [
'email' => $email,
'username' => $username,
Expand All @@ -94,7 +137,6 @@ public function registerAction(Request $request)
$violations->add($cause);
}
}

throw new ValidationException($violations);
}

Expand All @@ -103,7 +145,9 @@ public function registerAction(Request $request)
$user = $registration->toUser($this->userManager);

$violations = $this->validator->validate($user);

$this->registerLogger->info('violations data : violations={violations};', [
'violations' => $violations
]);
if (count($violations) > 0) {
throw new ValidationException($violations);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{% form_theme form 'bootstrap_3_layout.html.twig' %}
{#% form_theme form 'bootstrap_3_layout.html.twig' %#}

{% trans_default_domain 'NucleosProfileBundle' %}

Expand All @@ -13,7 +13,35 @@
{{ form_start(form, {'method': 'post', 'action': path('nucleos_profile_registration_register'), 'attr': {'class': 'nucleos_profile_registration_register'}}) }}
{{ form_row(form.email) }}
{{ form_row(form.username) }}
{% set break = false %}
{% for key, error in form.vars.errors if not break %}
{% if error.cause.propertyPath == 'username' %}
<div class="alert alert-danger">
<ul class="list-unstyled">
<li>
<span class="glyphicon glyphicon-exclamation-sign"></span>
{{ error.message }}
</li>
</ul>
</div>
{% set break = true %}
{% endif %}
{% endfor %}
Comment on lines +17 to +29
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed, the form itself should render the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't, There are 2 problems

  • Error bubbling cannot be activated as the form doesn't inherit, it is injected but also directly referenced from the vendor code. The configuration for validations seems to be not present and without inheriting, i don't know how can i configure error bubbling.
  • Validation also is not set up in a way that we can validate the password's security and choose which validations are active for the email and username (we want to prevent the "already used" message from showing)

{{ form_row(form.plainPassword) }}
{% set break = false %}
{% for key, error in form.vars.errors if not break %}
{% if error.cause.propertyPath == 'plainPassword' %}
<div class="alert alert-danger">
<ul class="list-unstyled">
<li>
<span class="glyphicon glyphicon-exclamation-sign"></span>
{{ error.message }}
</li>
</ul>
</div>
{% set break = true %}
{% endif %}
{% endfor %}

{% if form.legal is defined %}
{{ form_row(form.legal) }}
Expand All @@ -31,6 +59,20 @@
<div class="help-block">
{% trans from 'messages' %}authentication.unsubscribe{% endtrans %}
</div>
{% set break = false %}
{% for key, error in form.vars.errors if not break %}
{% if error.cause.propertyPath == 'email' %}
<div class="alert alert-danger">
<ul class="list-unstyled">
<li>
<span class="glyphicon glyphicon-exclamation-sign"></span>
{% trans from 'messages' %}authentication.registration_error{% endtrans %}
</li>
</ul>
</div>
{% set break = true %}
{% endif %}
{% endfor %}
{{ form_end(form) }}
<hr>
<div class="panel panel-default">
Expand Down
1 change: 1 addition & 0 deletions translations/messages.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,7 @@ authentication.forgot_password: Forgot password?
authentication.not_registered_yet: Not registered yet?
authentication.create_account: Create your account
authentication.already_registered: Already registered?
authentication.registration_error: There is a problem with the information you provided. Please review it and try again.
authentication.unsubscribe: You can easily unsubscribe at any time via the unsubscribe links in each of our emails
authentication.sign_in: Sign in
authentication.choose_password: Choose your password
Expand Down
1 change: 1 addition & 0 deletions translations/messages.es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,7 @@ authentication.forgot_password: ¿Contraseña olvidada?
authentication.not_registered_yet: ¿Todavía no estas registrado?
authentication.create_account: Crea tu cuenta
authentication.already_registered: ¿Ya tienes una cuenta?
authentication.registration_error: Hay un problema con tu registro. Revisa tus datos e intenta de nuevo por favor.
authentication.sign_in: Regístrate
maintenance.text: Sitio en mantenimiento. Volveremos pronto
api_apps.breadcrumb: Clientes API
Expand Down