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

Add start form for new registrations #695

Merged
merged 16 commits into from
Mar 4, 2020
Merged

Add start form for new registrations #695

merged 16 commits into from
Mar 4, 2020

Conversation

cintamani
Copy link
Contributor

@cintamani cintamani commented Feb 26, 2020

From: https://eaflood.atlassian.net/browse/RUBY-840

This adds a new page to the services which allow a user to start a new registration journey.
The page is part of the new registration workflow.

I suggest reviewing by separate commits

Screenshot 2020-02-26 at 15 22 42

Screenshot 2020-02-26 at 15 14 45

From: https://eaflood.atlassian.net/browse/RUBY-840

This adds a new page to the services which allow a user to start a new registration journey.
The page is part of the new registration workflow.
…ansient registration is of type new registration. In WEX, we do not check for the presence and format of a reference so we might later decide to remove this check entirely and move it to relevant start forms only.
…start form. We will need to do further refactoring on the way we authenticate users on each separate journey.
@cintamani cintamani added the enhancement New feature or request label Feb 26, 2020
@cintamani cintamani self-assigned this Feb 26, 2020
@Cruikshanks
Copy link
Member

@cintamani I've removed myself and @irisfaraway as reviewers as I know we suggested the tweak about prefixing the start option with temp plus assuming once the other PR's are merged this will undergo a bit of refactoring.

So figured it makes our TODO list a little clearer if we remove ourselves for now and then you add us again when the changes have been made.

@cintamani
Copy link
Contributor Author

Yes definitely! Thanks guys :) will ping you when it is ready again

@cintamani cintamani changed the title Add start form for new registrations [WIP] Add start form for new registrations Feb 28, 2020
@cintamani
Copy link
Contributor Author

cintamani commented Mar 2, 2020

The missing test coverage is for the temporary renewing registration forms and controller which will be implemented and properly tested as per RUBY-841

Hence I am going to ignore codeclimate's complain in this PR.

@cintamani cintamani changed the title [WIP] Add start form for new registrations Add start form for new registrations Mar 2, 2020
Copy link
Member

@irisfaraway irisfaraway left a comment

Choose a reason for hiding this comment

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

Looking good :) Just a few small questions and tweaks! I am not fussed about CodeClimate though as we know it'll be fixed when we actually make those pages.

app/views/waste_carriers_engine/start_forms/new.html.erb Outdated Show resolved Hide resolved
@@ -46,6 +46,7 @@ module WasteCarriersEngine
context "when the resource is not a transient_registration" do
Copy link
Member

Choose a reason for hiding this comment

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

Is this accurate? A NewRegistration is still a type of TransientRegistration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is not testing the validity of a new_regisration object in the base_form, but the validity of a registration object passed as a transient registration, so is still correct. You could argue that there is no test coverage for when we have a transient registration that is of type new registration, but as we discussed during the dev meeting we aren't sure about keeping this validation at all, and I will have to sort it out for validating the correct CBDU/CBDL numbers are generated later on so I just added that is_a? as a placeholder and make things pass now. We will need to go in more details about that in some other PR/meeting :D

Copy link
Member

Choose a reason for hiding this comment

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

So the first allow is making sure that is_a?(TransientRegistration) always returns false, but the second allow makes sure we still actually call is_a?(NewRegistration) and use the real response – am I understanding this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the second call is stubbing is_a?(NewRegistration) and making it return nil

spec/requests/waste_carriers_engine/start_forms_spec.rb Outdated Show resolved Hide resolved
Co-Authored-By: Iris Faraway <iris.faraway@gmail.com>
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

Just a thing about the naming of the start option attribute. Appreciate I've only highlighted it in one place but I know it's going to need to be updated in a few others.

app/models/waste_carriers_engine/new_registration.rb Outdated Show resolved Hide resolved
@Cruikshanks Cruikshanks merged commit c6d4de7 into master Mar 4, 2020
@irisfaraway irisfaraway deleted the 840-start-form branch April 6, 2020 15:16
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