mysociety / alaveteli

Provide a Freedom of Information request system for your jurisdiction
https://alaveteli.org
Other
389 stars 195 forks source link

Deprecate external requests #6893

Open garethrees opened 2 years ago

garethrees commented 2 years ago

External requests were added as part of Brighton's use of the now defunct foi-register.

External requests cause a load of complexity, and as we've no intention to use them further, should be removed https://github.com/mysociety/alaveteli/issues/6446.

At a high level, I think the plan is:

garethrees commented 2 years ago

Destroy test requests published on WDTK

There are 10s of requests published on WDTK that are tests – e.g. https://www.whatdotheyknow.com/request/how_many_security_breaches_in_th (note External url: | http://robin.foi-register.dev.mysociety.org/requests/26)

We can destroy any request where the external_url is obviously a test:

TEST_REQUEST_PATTERNS = %w[
  robin.foi-register.dev.mysociety.org
  brighton-hove.foi-register.staging.mysociety.org
]

InfoRequest.external.find_each do |request|
  puts "InfoRequest#id: #{request.id}"
  request.destroy! if TEST_REQUEST_PATTERNS.any? { |p| request.external_url =~ Regexp.new(p) }
end
garethrees commented 2 years ago

Add comment to each request explaining that it was made through foi-register, who it was made by (InfoRequest#external_user_name) and its external URL (InfoRequest#external_url)

There is at least one request where we don't have an external_url (it's an empty string), so we'll need to handle that.

SELECT id,external_url FROM info_requests WHERE external_url = '' AND external_user_name IS NOT NULL;
   id   | external_url
--------+--------------
 200581 |
 201231 |
 202129 |
(3 rows)

There are no external requests where the external_user_name is blank:

InfoRequest.external.where(external_user_name: '').count
# => 0
# Redefine InfoRequest#move_to_user to account for the request not having an
# `old_user`.
InfoRequest.class_eval do
  def move_to_user(destination_user, opts = {})
    return nil unless destination_user.try(:persisted?)
    # old_user = user
    editor = opts.fetch(:editor)

    attrs = {
      user: destination_user, external_user_name: nil, external_url: nil
    }

    return_val = if update(attrs)
      log_event('move_request',
                :editor => editor,
                :user_url_name => user.url_name,
                :old_user_url_name => nil)

      reindex_request_events

      user
    end

    # HACK: Manually reset counter caches
    # https://github.com/rails/rails/issues/10865
    user.class.reset_counters(user.id, :info_requests)
    return_val
  end
end

# Create prior to this step
anonymous_user = User.find_by(email: 'foi-register-external-user@invalid')
admin = User.internal_admin_user
commenter = User.find(201388) # WhatDoTheyKnow anonymous user

InfoRequest.external.each do |request|
  puts "Moving InfoRequest##{request.id}"
  author = request.external_user_name || "an anonymous user"

  comment = <<~EOF.squish
  This request was made by #{author} through mySociety's now defunct FOI Register
  software (https://www.mysociety.org/2014/02/10/brighton-and-hove-city-council-launch-new-foi-system-in-aim-to-become-more-transparent/).
  The integration has now been removed.
  EOF

  if request.external_url.present?
    comment << " The external URL for the request was #{request.external_url}."
  end

  request.add_comment(comment, commenter)

  request.move_to_user(anonymous_user, editor: admin)
end
garethrees commented 2 years ago

Purge the codebase of all external request handling

Doing this would mean dropping the ability to create new external requests through the API. https://github.com/mysociety/alaveteli/blob/0.40.1.0/app/controllers/api_controller.rb#L38-L39

gbp commented 2 years ago

This looks like a good plan of action.

External requests haven't been created since 2015-07-23 so this will be a good improvement to make and will simplify the codebase.

garethrees commented 2 years ago

This would fix https://github.com/mysociety/alaveteli/issues/6893.

garethrees commented 2 years ago

Create an anonymous user to re-home external requests

sha = Digest::SHA1.hexdigest(rand.to_s)
password = MySociety::Util.generate_token

about_me = <<~EOF
  Requests made through mySociety's now defunct FOI Register software have been transferred to this user, as the integration has now been removed.

  https://www.mysociety.org/2014/02/10/brighton-and-hove-city-council-launch-new-foi-system-in-aim-to-become-more-transparent/
EOF

User.create!(
  name: 'Anonymous User',
  email: 'foi-register-external-user@invalid',
  email_confirmed: true,
  url_name: sha,
  password: password,
  password_confirmation: password,
  about_me: about_me,
  receive_email_alerts: false,
  closed_at: Time.zone.now
)
garethrees commented 2 years ago
InfoRequest.external.count
# => 0

🎉

garethrees commented 2 years ago

During the migration of external requests to an anonymised user account I used the built in User.internal_admin_user method for the event log.

This special method uses the configured contact email address for the account email of the internal admin user. If it can’t find a user account with that email, it gets automatically created.

It looks like our original internal admin user currently has the @mysociety.org address as the account email, which resulted in a new user account being created with the @whatdotheyknow.com address configured in CONTACT_EMAIL.

Looks like it was changed almost two years ago [1] as part of some troubleshooting done to identify the cause of the internal admin user not receiving email alerts (https://github.com/mysociety/whatdotheyknow-theme/issues/931).

We're now good to change it back.

I've swapped the email addresses for the accounts and marked the newest internal admin user account as closed.

I won't bother updating the editor attribute in the InfoRequestEvent#params_yaml as that serializes the entire User object.

[1] rfc822msgid:<[CAEu8bMmHyGzuTA4mCxkWgnrK6rEw+pva89rgsOnFHOatozF9WQ@mail.gmail.com](mailto:CAEu8bMmHyGzuTA4mCxkWgnrK6rEw%2Bpva89rgsOnFHOatozF9WQ@mail.gmail.com)>

garethrees commented 1 year ago

External requests now cause errors in development mode (particularly in the admin ui) because the info request both_links doesn't work with external requests.

Quick fix is to destroy the fixtures:

rails runner 'InfoRequest.external.destroy_all'