Merge Request #57

← To merge requests From registration_errors into master
Merged
noosfero-themes/angular-theme!57
Created by Daniela Feitosa

Enhancements on RegisterComponent 

* Move error messages to shared folder
* Refactor and addition of tests for signup on RegisterComponent
* Display error messages after sumbit registration
* Add new strings on translations files
* Add login link on registration page
Assignee: Caio Almeida
Milestone: None

Merged by Victor Costa

Source branch has been removed
Commits (1)
  • ecca2061   Enhancements on RegisterComponent ... Browse Code »
    1 
    Also:
* Move error messages to shared folder
* Refactor and addition of tests for signup on RegisterComponent
* Display error messages after sumbit registration
* Add new strings on translations files
* Add login link on registration page
    Daniela Feitosa
     
5 participants
  • 0743c2eb05e68c92baa5f8c498995a20?s=40&d=identicon
    Link here  
    Daniela Feitosa @daniela

    @caiosba @mfdeveloper: esse MR é uma continuação do !47

    Choose File ...   File name...
    Cancel
  • Me
    Link here  
    Michel Felipe @mfdeveloper (Edited )

    Perfeito @daniela Irei revisá-lo :)

    Choose File ...   File name...
    Cancel
    • Me
    Show/hide discussion
    Michel Felipe started a discussion on the diff
    last updated by Michel Felipe
    src/app/account/register-component.html
    17 25 <span class="input-group-addon" style="font-weight: bold;"><i class="fa fa-globe"></i>&nbsp;{{ ctrl.environment.host }}</span>
    18 26 <input type="text" id="username" name="username" class="form-control" placeholder="{{'account.register.usernameLabel' | translate }}" ng-model="ctrl.account.login" required>
    19 27 </div>
    20   - <div class="help-block" ng-show="signupForm.username.$touched" ng-messages="signupForm.username.$error">
    21   - <ul class="list-unstyled">
    22   - <li ng-messages-include="languages/messages.html"></li>
    23   - </ul>
      28 + <div class="help-block" ng-messages="signupForm.username.$error">
      29 + <div ng-if="signupForm.username.$touched && !signupForm.username.$empty">
      30 + <ul class="list-unstyled">
      31 + <li ng-messages-include="app/shared/messages/form-errors.html"></li>
    1
    • Me
      Link here  
      Michel Felipe @mfdeveloper (Edited )

      @daniela Por que o arquivo .html contendo as mensagens de erro foram movidas? Essa pasta shared é um novo local para arquivos desse gênero? Vocês discutiram isso internamente? Esse tipo de abordagem é interessante, mas sugiro a atualização do README.md informando quais as arquivos devem estar nessa pasta, para que fique claro para todos os envolvidos :)

      Choose File ...   File name...
      Cancel
    Me
    Show/hide discussion
    Michel Felipe started a discussion on the diff
    last updated by Michel Felipe
    src/app/account/register.component.ts
    63 72 scope: this.$scope
    64 73 });
    65 74 }
      75 +
      76 + openLogin() {
    1
    • Me
      Link here  
      Michel Felipe @mfdeveloper (Edited )

      O código deste método parece estar duplicado com a definição em Navbar#openLogin() nao? Acredito que seria melhor acessar o componente Navbar, ou até mesmo mover esse método para a classe AuthController, para que seja acessível e reutilizável entre os dois componentes. O que você acha @daniela ?

      Choose File ...   File name...
      Cancel
    Me
    Show/hide discussion
    Michel Felipe started a discussion on the diff
    last updated by Michel Felipe
    src/languages/en.json
    112 112 "account.register.signupMessage": "Register",
    113 113 "account.register.haveAccountMessage": "Already have an account?",
    114 114 "account.register.termsOfUseMessage": "terms of use",
      115 + "account.register.termsOfUseTitle": "Terms of use",
    1
    • Me
      Link here  
      Michel Felipe @mfdeveloper (Edited )

      Parece que há uma duplicidade de valores entre as chaves account.register.termsOfUseMessage e account.register.termsOfUseTitlenao? A única coisa diferente entre os seus valores é o "T" em caixa alta. Essas duas chaves são realmente necessárias?

      Choose File ...   File name...
      Cancel
    Me
    Show/hide discussion
    Michel Felipe started a discussion on the diff
    last updated by Michel Felipe
    src/languages/pt.json
    122 123 "messages.invalid.maxlength": "O valor é muito longo",
    123 124 "messages.invalid.minlength": "O valor é muito curto",
    124 125 "messages.invalid.email": "Informe um email válido",
    125   - "messages.invalid.passwordMatch": "As senhas não coincidem"
      126 + "messages.invalid.passwordMatch": "As senhas não coincidem",
      127 + "account.register.save.failed": "A conta não pode ser criada",
      128 + "account.register.mandatoryField": "Este campo é obrigatório",
      129 + "account.register.invalidEmail": "Este campo deve ser um endereço de e-mail válido.",
      130 + "account.register.shortPassword": "A senha deve ter no mínimo 4 caracteres",
      131 + "account.register.errorMessagesTitle": "Houve erros com os seguintes campos:",
      132 + "email is missing": "email não pode ficar em branco",
      133 + "login is missing": "nome de usuário não pode ficar em branco",
      134 + "password is missing": "senha não pode ficar em branco",
      135 + "login": "nome de usuário",
      136 + "has already been taken": "já está sendo usado",
      137 + "email": "email",
    1
    Me
    Show/hide discussion
    Michel Felipe started a discussion on the diff
    last updated by Michel Felipe
    src/app/account/register.component.ts
    42   - } else {
    43   - throw new Error('Invalid attributes');
    44   - }
    45   - });
    46   - } else {
    47   - this.notificationService.error({ message: "account.register.passwordConfirmation.failed" });
    48   - }
      37 + let error = '';
      38 + let errors: any;
      39 + let field = '';
      40 + this.$scope.errorMessages = [];
      41 + this.registerService.createAccount(this.account).then((response) => {
      42 + this.$state.transitionTo('main.environment');
      43 + this.notificationService.success({ title: "account.register.success.title", message: "account.register.success.message" });
      44 + }).catch((response) => {
      45 + if ( response.data.error ) {
    1
    • Me
      Link here  
      Michel Felipe @mfdeveloper (Edited )

      A utilização deste atributo errorMessages contendo os erros gerados pelo backend é realmente a melhor solução? Vocês discutiram internamente em equipe acerca disso @daniela ? Observe que, uma vez que é chamado simplesmente um catch() da Promise resultante da requisição REST, mensagens de erros relacionadas a status como 500, 503 entre outros serão exibidas para o usuário correto?

      Além disso, uma vez que a validação já é controlada pelo ngMessages, faz sentido requisitar o servidor para obter "campos requeridos" por exemplo? È bem verdade que é configurável no Noosfero quais campos são ou não obrigatórios no registro de usuário, e que "replicar" essas verificações no front-end parece uma duplicação da regra de negócio, mas de qualquer forma, o usuário teria as duas mensagens exibidas, tanto nos campos quanto acima do form, com o novo HTML adicionado por esse MR.

      Em suma, qual a avaliação de vocês sobre essas questões? Questiono isso para pensarmos juntos realmente xD

      Choose File ...   File name...
      Cancel
    Me
    Show/hide discussion
    Michel Felipe started a discussion on the diff
    last updated by Michel Felipe
    src/app/shared/components/interfaces.ts
    6 6 $uibModal: ng.ui.bootstrap.IModalService;
    7 7 modalInstance: ng.ui.bootstrap.IModalServiceInstance;
    8 8 }
      9 +
      10 +export interface IErrorMessages extends ng.IScope {
    1
    • Me
      Link here  
      Michel Felipe @mfdeveloper

      Pq criar uma interface só para isso? errorMessages não poderia simplesmente ser um atributo do componente RegisterComponent? Observe que os atributos de um componente são inseridos automaticamente no $scope, uma vez q o ngForward faz esse de => para

      Choose File ...   File name...
      Cancel
    Me
    Show/hide discussion
    Michel Felipe started a discussion on the diff
    last updated by Michel Felipe
    src/app/account/register.component.spec.ts
    45 41 helper = new ComponentTestHelper<RegisterComponent>(cls, done);
    46 42 });
    47 43  
      44 + beforeEach(inject((_$rootScope_: ng.IRootScopeService, _$q_: ng.IQService) => {
    1
    • Me
      Link here  
      Michel Felipe @mfdeveloper

      Esse inject() é somente para injetar os serviços do angular? Se sim, existe uma classe AngularServiceFactory que contém o método getAngularService() voltado para esse tipo de "injeção". Salvo em algumas exceções, a idéia é tentar mitigar o máximo possível chamadas explicitas a elementos específicos do Angular 1.x, uma vez que estamos utilizando ngForward

      Choose File ...   File name...
      Cancel
    Me
    Show/hide discussion
    Michel Felipe started a discussion on the diff
    last updated by Michel Felipe
    src/app/account/register.component.spec.ts
    48 49 it('register page was rendered', () => {
    49 50 expect(helper.debugElement.query('div.register-page').length).toEqual(1);
    50 51 });
    51 52  
      53 + it("registers a new user", done => {
      54 + user_data = { username: "username", password: "password", password_confirmation: "password", email: "user@company.com" };
      55 + response = { };
      56 +
      57 + helper.component.account = user_data;
      58 +
      59 + deferred = $q.defer();
      60 + deferred.resolve({ data: response });
      61 + registerService.createAccount = jasmine.createSpy("createAccount").and.returnValue(deferred.promise);
      62 +
      63 + helper.component.signup();
      64 + $rootScope.$apply();
    1
    • Me
      Link here  
      Michel Felipe @mfdeveloper

      È realmente necessário uma chamada explicita a $apply()? A classe do ngForward chamada ComponentFixture possui um método detectChanges(), que no final das contas faz uma chamada a $apply() por debaixo dos panos. Como vc está utilizando a classe ComponentTestHelper nesses testes, nela existe o método detectChanges() que é um "atalho". Verifique se isso atende ao que você precisa :)

      Choose File ...   File name...
      Cancel
    Me
    Show/hide discussion
    Michel Felipe started a discussion on the diff
    last updated by Michel Felipe
    src/app/account/register.component.spec.ts
      65 +
      66 + expect(registerService.createAccount).toHaveBeenCalledWith(user_data);
      67 + expect(stateService.transitionTo).toHaveBeenCalledWith("main.environment");
      68 + expect(notificationService.success).toHaveBeenCalled();
      69 +
      70 + done();
      71 + });
      72 +
      73 + it("gives error when registration fails", done => {
      74 + user_data = { password: "pas" };
      75 + response = { data: { message: '{ "password": ["is too short"] }'}};
      76 +
      77 + helper.component.account = user_data;
      78 +
      79 + deferred = $q.defer();
      80 + deferred.reject(response);
    1
    • Me
      Link here  
      Michel Felipe @mfdeveloper

      Uma chamada direta a Promise.reject() não funcionaria ao invés de injetar o serviço $q?

      Choose File ...   File name...
      Cancel
  • C8b72d0556872a2aea21e8fed0a72001?s=40&d=identicon
    Link here  
    Melissa Wen @melissawen

    mentioned in issue #138

    Choose File ...   File name...
    Cancel
  • 4a20548511a65cfccc863520b70c3ee9?s=40&d=identicon
    Link here  
    Victor Costa @vfcosta

    Merged manually, thanks!

    Choose File ...   File name...
    Cancel
    • 0743c2eb05e68c92baa5f8c498995a20?s=40&d=identicon
    Show/hide discussion
    Daniela Feitosa started a discussion on commit ecca2061
    last updated by Daniela Feitosa
  • 0743c2eb05e68c92baa5f8c498995a20?s=40&d=identicon
    Link here  
    Daniela Feitosa @daniela

    mentioned in issue #138

    Choose File ...   File name...
    Cancel