1791 enforce full domain in site settings #2201
Conversation
|
Looking at the PR and the issue I think the issue is incorrect: the field is called "domain" and is intended to be just a URL domain, which doesn't include the http bit or anything after the query string. |
|
change domain to url so clear what it's being used for. do migration. |
app/models/site.rb
Outdated
| @@ -36,6 +36,7 @@ class Site < ApplicationRecord | |||
| validates :name, :slug, :domain, presence: true | |||
| validates :place_name unless :default_site? | |||
| validates :hero_text, length: { maximum: 120 } | |||
| validates :domain, format: { with: %r{\Ahttps://[^\s,]+\z}, message: 'A domain must start with "https://"' } | |||
There was a problem hiding this comment.
It looks like this domain field is a really important part for how sites work!
here we use domain to find the site from the request.
Okay (after discussing this) it looks like we don't want to use the domain this way any more, so maybe refactor Site.find_by_request method to not reference the domain and update any tests covering that logic.
There was a problem hiding this comment.
Maybe also rename the field to URL (or site_url if that is too generic a name)
| @@ -16,7 +16,7 @@ class SiteType < Types::BaseObject | |||
| null: false, | |||
| description: 'Short unique URL friendly version of name' | |||
|
|
|||
| field :domain, String, | |||
| field :url, String, | |||
There was a problem hiding this comment.
This is probably okay but its worth bearing in mind that outside people may rely on our public API and so we should not change the name in a breaking way for them. Let's just alias domain to url for now
|
|
||
| class ChangeDomainToUrl < ActiveRecord::Migration[6.1] | ||
| def self.up | ||
| rename_column :sites, :domain, :url |
There was a problem hiding this comment.
you can actually just use the one method here self.change and rails will figure out the inverse of the rename depending if you are migrating up or down on its own (not a suggestion, just a comment for future)
to not break the api
|
@ivan-kocienski-gfsc hopefully this resolves those last issues |
|
I should also say I rebuilt transdimension elm app locally with no issues, I looked at the data folder and I think we just pull in partners based on the (partnership)tag not any site rule, probably because it covers the whole uk so geographic info is redundant. |
fixes #1791
A small fix that required a lot of test wrangling