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 new transition form #1384

Merged
merged 11 commits into from
Sep 11, 2023
Merged

Add new transition form #1384

merged 11 commits into from
Sep 11, 2023

Conversation

jkempster34
Copy link
Contributor

@jkempster34 jkempster34 commented Sep 5, 2023

Note

This is not the finished article, this is for us to refine as we learn more about how it will be used.

https://trello.com/c/3NYAPZs4

Add constants for global and special redirect types

We want to add a new site form. In order to do this we will need an easy way to
access these values.

We might want to consider refining these further into enums, as this would
allow us to remove some of the methods in the site model.

Add routes for site creation

We want to add sites per organisation.

This add nested routes for site creation.

Add site form object

We are aiming to archive Transition Config which is currently used to create
sites in Transition.

This adds a form object for the creation of a site.

It's worth noting that the process currently moves existing hosts and aka hosts
to the new site if they already exist (see
lib/transition/import/site_yaml_file.rb). We aren't yet sure whether we want
to create a new UI for that functionality, or to use this form. For now, this
form does not have that functionality.

Only validate presence of canonical host or aka if hostname is present

If hostname is not provided for the host, this validation will raise an error.

Add a condition that this validation only runs when hostname is present.

Add site creation actions and views

We are aiming to archive Transition Config which is currently used to create
sites in Transition.

This adds the actions and views necessary for a user to add a new transition.

Validate that host hostname is lowercase

Currently the site importer (lib/transition/import/site_yaml_file.rb)
downcases any hostnames.

This adds validation to the host model which will be surfaced on the new site
form.

Only allow Site managers to create new sites

This adds authorization that only people with the Site manager role in GOV.UK
Signon can create new sites.

This is so that we can safely test new features before we officially archive
Transition Config.

Validate that global new URL is absent if the global type is "archive"

Currently, there is validation in place to prevent the creation of sites which
have a global type of "redirect" without a global new URL, but the opposite
validation is missing.

Improve appearance of radio buttons

In order to style these using Bootstrap, we need to provide a block to collection_radio_buttons.

Nilify blanks on the Site model

The optional fields of the site form come through as empty strings when left
blank. These are then saved in the database, when ideally these would be saved
as nil in order to preserve the previous behaviour of adding them through
Transition Config for consistency.

We already have a module that handles the nilification of blanks, however we
need to make a couple of changes:

  • We add an "except list" of attributes that we don't want to nilify, such as
    Booleans that should be false and not nil (false.blank? == true).
  • We allow blank for special_redirect_strategy as this will now be nilified
    before save.

Add hints to new site form inputs

This copies the guidance fro Transition Config over to the new site form.

This will need later refinement.

image


This application is owned by the publishing platform team. Please let us know in #govuk-publishing-platform when you raise any PRs.

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

@jkempster34 jkempster34 force-pushed the add-new-transition-form branch 10 times, most recently from 6f6e253 to acb57c5 Compare September 5, 2023 18:08
@@ -12,6 +12,7 @@ class Host < ApplicationRecord
validates :hostname, hostname: true
validates :site, presence: true
validate :canonical_host_id_xor_aka_present, if: -> { hostname.present? }
validate :hostname_is_downcased, if: -> { hostname.present? }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just not downcase the host before we send it on?

@JonathanHallam
Copy link
Contributor

Really like this, thanks Joe, only comment is wondering whether there's a way we can let people know what all of the fields are and maybe give a link for the tna timestamp section? Maybe lets have a chat offline?

@jkempster34 jkempster34 force-pushed the add-new-transition-form branch 3 times, most recently from 12d54d7 to 32639cb Compare September 8, 2023 09:54
We want to add a new site form. In order to do this we will need an easy way to
access these values.

We might want to consider refining these further into enums, as this would
allow us to remove some of the methods in the site model.
We want to add sites per organisation.

This add nested routes for site creation.
If hostname is not provided for the host, this validation will raise an error.

Add a condition that this validation only runs when hostname is present.
We are aiming to archive Transition Config which is currently used to create
sites in Transition.

This adds a form object for the creation of a site. 

It's worth noting that the process currently moves existing hosts and aka hosts
to the new site if they already exist (see
`lib/transition/import/site_yaml_file.rb`). We aren't yet sure whether we want
to create a new UI for that functionality, or to use this form. For now, this
form does not have that functionality.
We are aiming to archive Transition Config which is currently used to create
sites in Transition.

This adds the actions and views necessary for a user to add a new transition.
Currently the site importer (`lib/transition/import/site_yaml_file.rb`)
downcases any hostnames.

This adds validation to the host model which will be surfaced on the new site
form.
This adds authorization that only people with the `Site manager` role in GOV.UK
Signon can create new sites.

This is so that we can safely test new features before we officially archive
Transition Config.
Currently, there is validation in place to prevent the creation of sites which
have a global type of "redirect" without a global new URL, but the opposite
validation is missing.
In order to style these using Bootstrap, we need to provide a block to `collection_radio_buttons`.
The optional fields of the site form come through as empty strings when left
blank. These are then saved in the database, when ideally these would be saved
as `nil` in order to preserve the previous behaviour of adding them through
Transition Config for consistency.

We already have a module that handles the nilification of blanks, however we
need to make a couple of changes:
- We add an "except list" of attributes that we don't want to nilify, such as
  Booleans that should be false and not nil (false.blank? == true).
- We allow blank for `special_redirect_strategy` as this will now be nilified
  before save.
This copies the guidance fro Transition Config over to the new site form.

This will need later refinement.
@jkempster34 jkempster34 force-pushed the add-new-transition-form branch from 32639cb to 1f8f4ef Compare September 8, 2023 10:36
Copy link
Contributor

@JonathanHallam JonathanHallam left a comment

Choose a reason for hiding this comment

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

Given the caveat of improving this as we go on, I'm happy with this for now, great work :)

@jkempster34 jkempster34 merged commit 947d3d4 into main Sep 11, 2023
5 checks passed
@jkempster34 jkempster34 deleted the add-new-transition-form branch September 11, 2023 15:01
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.

2 participants