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.

Sign up for GitHub

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

Jump to bottom

[#6434] Exclude email providers from body home pages #7836

Merged
merged 5 commits into from
Oct 19, 2023
Merged

[#6434] Exclude email providers from body home pages #7836

merged 5 commits into from
Oct 19, 2023

Conversation

garethrees
Copy link
Member

@garethrees garethrees commented Jul 21, 2023 •
edited
Loading

Many smaller authorities use a request email address from a general
purpose public email provider. We don't want e.g. gmail.com being set as
the home page for the authority.

This is configurable in the theme by setting the class attribute:

# Add to the defaults
PublicBody.excluded_calculated_home_page_domains << %w[
  example.net
]

# Clear the defaults and set your own list
PublicBody.excluded_calculated_home_page_domains = %w[
  example.org
  example.net
]

Fixes #6434

@garethrees
Copy link
Member Author

Any others we should add as defaults for Alaveteli? Can't cover every email provider but should include the most common international ones.

@garethrees
Copy link
Member Author

Might want to show appending domains for configuration too:

PublicBody.excluded_calculated_home_page_domains << %w[
  example.org
  example.net
]

@HelenWDTK
Copy link
Contributor

HelenWDTK commented Jul 21, 2023 •
edited
Loading

Any others we should add as defaults for Alaveteli? Can't cover every email provider but should include the most common international ones.

gmail, outlook, aol, gmx, mail.com, hotmail, iCloud, proton, yahoo. - QQ, zoho, yandex and mail.ru are big but a bit more region specific.

@mysociety-pusher mysociety-pusher force-pushed the 6434-excluded-calculated-homepage-domains branch 5 times, most recently from 1e1243c to e811f1e Compare July 24, 2023 11:57
@garethrees garethrees marked this pull request as ready for review July 24, 2023 11:58
mdeuk
mdeuk reviewed Jul 24, 2023
View reviewed changes
Copy link
Collaborator

@mdeuk mdeuk left a comment

Choose a reason for hiding this comment

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

The list of domains makes sense - but, a scan of the wdtk database suggests some additions would be helpful 1.

It seems to make sense to put them here, rather than branch them out on wdtk-theme, as they are possibly also of use to others.

Footnotes

  1. Primarily UK ISPs, but also the uk variants of Hotmail. Sadly, we have loads of them, primarily in small Government organisations. ↩

app/models/public_body/calculated_home_page.rb Show resolved Hide resolved
app/models/public_body/calculated_home_page.rb Show resolved Hide resolved
app/models/public_body/calculated_home_page.rb Outdated Show resolved Hide resolved
app/models/public_body/calculated_home_page.rb Show resolved Hide resolved
app/models/public_body/calculated_home_page.rb Show resolved Hide resolved
@garethrees
Copy link
Member Author

The list of domains makes sense - but, a scan of the wdtk database suggests some additions would be helpful

It seems to make sense to put them here, rather than branch them out on wdtk-theme, as they are possibly also of use to others.

Thanks, I'll add these variants. 🙌 Might pop the .co.uk variants on the theme; will have a think. It's a bit of an arms race. There are libraries that track these free mail providers, so might consider using something like that should it start getting out of hand. I'm mainly interested in the most common cases, since ones where we only have a handful can be resolved by adding the actual home page.

gbp
gbp approved these changes Sep 21, 2023
View reviewed changes
Copy link
Member

@gbp gbp left a comment

Choose a reason for hiding this comment

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

This all looks good to me. Happy with the suggestions or we can bump some of those over to the theme.

@garethrees
Clean up PublicBody#calculated_home_page_specs
6fce404 
* Group related specs (adding protocol, then guessing based on domain)
* Quote style
* Missing parenthesis
* Set attribute on initialisation
* Remove “should” from spec descriptions
@garethrees
Extract PublicBody::CalculatedHomePage
bc83c78 
Going to later add a list of domains to exclude from the calculation so
moving to a module to contain related logic.
@garethrees
Exclude email providers from body home pages
5cc84cd 
Many smaller authorities use a request email address from a general
purpose public email provider. We don't want e.g. gmail.com being set as
the home page for the authority.

This is configurable in the theme by setting the class attribute:

    # Add to the defaults
    PublicBody.excluded_calculated_home_page_domains << %w[
      example.net
    ]

    # Clear the defaults and set your own list
    PublicBody.excluded_calculated_home_page_domains = %w[
      example.org
      example.net
    ]

Fixes #6434
@garethrees
Add PublicBody#calculated_home_page!
8aa79ac 
Removes the TODO by adding a cached and non-cached version of
PublicBody::CalculatedHomePage#calculated_home_page.

In current use it's safe to always use the cached version.

In future we might want to modify the non-bang version such that it
recalculates if the request_email attribute gets changed.
@garethrees
Refactor PublicBody::CalculatedHomePage#calculated_home_page
de37074 
Extract each piece of logic to own method to improve clarity.
@garethrees garethrees force-pushed the 6434-excluded-calculated-homepage-domains branch from e811f1e to de37074 Compare September 29, 2023 16:19
garethrees added a commit to mysociety/whatdotheyknow-theme that referenced this pull request Sep 29, 2023
@garethrees
Add UK-specific excluded calculated homepage domains
32ca854 
Requires mysociety/alaveteli#7836
@garethrees garethrees mentioned this pull request Sep 29, 2023
Merged
@garethrees
Copy link
Member Author

I've refined this PR so that it covers international providers and made a counterpart PR for UK domains at theme level mysociety/whatdotheyknow-theme#1789

@gbp
Copy link
Member

gbp commented Oct 19, 2023

@garethrees I'm going to get this merged unless there is any reason not to have this in #6676?

@garethrees
Copy link
Member Author

If we can we should run it in prod for a bit this morning just to check it all seems okay, but in general I don't see why not. 🚀

@gbp gbp merged commit 6ad629c into develop Oct 19, 2023
5 checks passed
gbp pushed a commit to mysociety/whatdotheyknow-theme that referenced this pull request Oct 19, 2023
@garethrees @gbp
Add UK-specific excluded calculated homepage domains
9f91e1c 
Requires mysociety/alaveteli#7836
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdeuk mdeuk mdeuk left review comments

@gbp gbp gbp approved these changes

Assignees
No one assigned
Labels
awaiting-deploy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prevent 'calculated homepage' from being generated for certain domains
4 participants
@garethrees @HelenWDTK @gbp @mdeuk