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

(128) Activity form uses govuk form builder #90

Merged
merged 7 commits into from
Dec 17, 2019

Conversation

lozette
Copy link
Contributor

@lozette lozette commented Dec 16, 2019

Changes in this PR

Trello: https://trello.com/c/AvIykhu2/128-refactor-activity-form-to-use-new-form-builder

All forms in the application now use govuk_design_system_formbuilder.

Next steps

  • Is an ADR required? An ADR should be added if this PR introduces a change to the architecture.
  • Is a changelog entry required? An entry should always be made in CHANGELOG.md, unless this PR is a small tweak which has to impact outside the development team.
  • Do any environment variables need amending or adding?

@lozette lozette force-pushed the refactor/128-activity-form-uses-new-form-builder branch 3 times, most recently from 8b38626 to db8b253 Compare December 16, 2019 16:08
Copy link
Contributor

@tahb tahb left a comment

Choose a reason for hiding this comment

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

Noticed we're missing some indication when a field is required, this used to be a leading asterisk I think.

Screenshot 2019-12-16 at 16 30 47

Apart from that this looks good to me 👍 Happy to be back on one form builder.

@@ -3,6 +3,10 @@ class Activity < ApplicationRecord
validates_presence_of :identifier
validates_uniqueness_of :identifier

attribute :recipient_region, :string, default: "998"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these defaults will work fine for fund activities but I wonder how we'll be able to support these being changed for programmes and projects, any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly a set of env variables?

Or maybe a helper along the lines of ActivityHelper. Although if the values needed to change than env variables are probably more flexible?

Copy link
Contributor

@tahb tahb Dec 16, 2019

Choose a reason for hiding this comment

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

I hadn't thought of environment variables, how might that look?

I wonder if having some initializer helpers might help. Something like:

class Activity

def setup_hierarchy_defaults
  case hierachy.class.name
  when fund setup_fund_defaults
…
end

private def setup_fund_defaults
  self.recipient_region = "998"
  …
end

private def setup_programme_detaults
  self.recipient_region = "2"
  …
end
class Staff::ActivitiesController < Staff::BaseController
  include ActivityHelper
  def create 
    @activity.hierarchy = hierarchy
    @activity.set_up_hierachy_defaults
    @activity.save(validate: false)
…

If we can't see a way to extend the current attribute implementation, I think it may be best to lose as it would be a secondary way of setting defaults which makes it conceptually harder to follow and opens us up to accidentally applying the wrong defaults at the wrong level of hierarchy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the initialiser helpers more, env variables would be clunky and I don't think these values are going to change often (maybe?)

@@ -1,15 +1,6 @@
# frozen_string_literal: true

module CodelistHelper
# TODO: Remove `yaml_to_options` when all forms are migrated from simple form to the govuk form builder,
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to see us closing the loop and binning the old version 👍

@lozette
Copy link
Contributor Author

lozette commented Dec 16, 2019

I think the asterisk for a required field is not part of the govuk design standard: https://govuk-elements.herokuapp.com/form-elements/#form-optional-fields

@tahb
Copy link
Contributor

tahb commented Dec 16, 2019

@lozette Ah brilliant 👌 Really what we'd need if anything would be to add (optional) in everywhere else however this card should add validation to everything so there's no point!

@lozette lozette force-pushed the refactor/128-activity-form-uses-new-form-builder branch 4 times, most recently from 22395a3 to b4263ce Compare December 17, 2019 12:52
leeky and others added 7 commits December 17, 2019 12:52
- Use GOVUK design system form builder and helpers
- Move setting defaults into the Activity model
- Move model translations to models_and_forms.en.yml
…jects

The latter is better supported by the new govuk forms
The defaults on the Activity model are Fund-specific. This change refactors
the setting of default attributes so when we add other hierarchy types, we can
add other defaults.
@lozette lozette force-pushed the refactor/128-activity-form-uses-new-form-builder branch from b4263ce to db04f3c Compare December 17, 2019 12:53
Copy link
Contributor

@tahb tahb left a comment

Choose a reason for hiding this comment

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

Worked on this together, LGTM.

@lozette lozette merged commit 5e7231d into develop Dec 17, 2019
@lozette lozette deleted the refactor/128-activity-form-uses-new-form-builder branch December 17, 2019 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants