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 change name form, routes, controller #56

Merged
merged 4 commits into from
Jun 20, 2022

Conversation

thomasiles
Copy link
Contributor

The current code contains logic for changing the form name along with
form submission email.

This commit builds on the name changing code, with the aim of making it
more production ready.

The following changes have been made:

  • splitting the form name change into it's own screen, inline with with
    prototype
  • Adds validation, updates page titles on errors and follows the design
    system
  • Adds a simple 404 page when a non-existent form is edited
  • Moves text strings into locale files for consistency and ease of
    updating
  • Adds a simple unit test
  • Adds more structure, using the form object
    pattern

    and a new Forms:: namespace to keep the size of the model/controllers
    in check
  • forms controller broken out a bit into features, with a
    base_controller to cover the common functionality
  • Adds 'govuk-components' to make future forms easier to implement

Most of this work is adding more structure to the project to keep
complexity down for future features. This can seem like overkill for
changing the name of a form, but things get hairy quite quickly without
it. Form Objects help keep validation code etc. outside of the model
object and help split controllers up.

There is a bit of an issue with naming - form objects are used to build the
user's form - I'm not sure there is a better name for either concept.
Adding Pages will make things even more fun.

There is still work to do:

  • Improve the unit testing - only a single case is covered. Further
    tests would mock the Form model to check things are updated as
    expected
  • Add e2e tests, probably using the existing work on cypress
  • Update the submission email route to match the name change and clean
    the current forms_controller up a bit
  • Improve routing
  • Logging

What problem does the pull request solve?

Checklist

  • I've used the pull request template
  • I've linked this PR to the relevant issue (if mission work)
  • I've written unit tests for these changes (if code change)
  • I've updated the documentation in (If any documentation requires updating)
    • README.md
    • Elsewhere (please link)

@thomasiles thomasiles requested a review from a team as a code owner June 16, 2022 12:22
@thomasiles thomasiles linked an issue Jun 16, 2022 that may be closed by this pull request
The current code contains logic for changing the form name along with
form submission email.

This commit builds on the name changing code, with the aim of making it
more production ready.

The following changes have been made:

- splitting the form name change into it's own screen, inline with with
  prototype
- Adds validation, updates page titles on errors and follows the design
  system
- Adds a simple 404 page when a non-existent form is edited
- Moves text strings into locale files for consistency and ease of
  updating
- Adds a simple unit test
- Adds more structure, using the [form object
  pattern](https://jaryl.medium.com/disciplined-rails-form-object-techniques-patterns-part-1-23cfffcaf429)
  and a new Forms:: namespace to keep the size of the model/controllers
  in check
- forms controller broken out a bit into features, with a
  base_controller to cover the common functionality
- Adds 'govuk-components' to make future forms easier to implement

Most of this work is adding more structure to the project to keep
complexity down for future features. This can seem like overkill for
changing the name of a form, but things get hairy quite quickly without
it. Form Objects help keep validation code etc. outside of the model
object and help split controllers up.

There is a bit of an issue with naming - form objects are used to build the
user's form - I'm not sure there is a better name for either concept.
Adding Pages will make things even more fun.

There is still work to do:
- Improve the unit testing - only a single case is covered. Further
  tests would mock the Form model to check things are updated as
  expected
- Add e2e tests, probably using the existing work on cypress
- Update the submission email route to match the name change and clean
  the current forms_controller up a bit
- Improve routing
- Logging
@thomasiles thomasiles force-pushed the 55-form-builder-can-edit-their-form-title branch from b63eedf to b948137 Compare June 16, 2022 12:28
Copy link
Contributor

@danielburnley danielburnley left a comment

Choose a reason for hiding this comment

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

Looks good so far - Probably want some request specs for the controllers. We can use the ActiveResource mocking for those and just make sure it submits the correct data. You can check the requests for posting using https://rubydoc.info/gems/activeresource/ActiveResource%2FHttpMock%2Erequests

@danielburnley danielburnley force-pushed the 55-form-builder-can-edit-their-form-title branch from 9c3c59d to 818f744 Compare June 17, 2022 16:02
@danielburnley danielburnley force-pushed the 55-form-builder-can-edit-their-form-title branch from 818f744 to 1524d03 Compare June 17, 2022 16:05
Copy link
Contributor

@DavidBiddle DavidBiddle left a comment

Choose a reason for hiding this comment

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

looks good to me 🎉

@thomasiles thomasiles merged commit 11a2b46 into main Jun 20, 2022
@danielburnley danielburnley deleted the 55-form-builder-can-edit-their-form-title branch June 20, 2022 08:23
@DavidBiddle DavidBiddle mentioned this pull request Jun 22, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Form builder can edit their form title
3 participants