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

Partner Profile : clean up social media, request types and pick up emails #4829

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

cielf
Copy link
Collaborator

@cielf cielf commented Dec 4, 2024

Required before #4801

Description

This cleans up the partner profiles so they will be valid

  • if nothing in the social media area, and social media is required, sets "no social media"
  • if no request types are enabled, enables all from the organization
  • removes invalid email if obviously meant to none (e.g. "n/a"),
  • converts email separators to ","
  • if email still not valid, appends to name, as " email: [remaining email], and blanks the email

Ran against production data with puts in place to check reasonablilty of that last step

Include anything else we should know about. -->

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Ran against production copy.

if pick_up == "none" or pick_up == "na" or pick_up == "n/a" or pick_up == "see above"
profile.pick_up_email = ""
else
profile.pick_up_email.sub!("/",",")
Copy link
Collaborator

@coalest coalest Dec 4, 2024

Choose a reason for hiding this comment

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

Do any of the pick_up_emails have multiple forward slashes or semicolons? If so, sub! would only remove the first one. Is that intended or would gsub! be safer here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it would. will fix tomorrow

# ActiveRecord::Base.logger = Logger.new(STDOUT)
invalid_profiles = Partners::Profile.all.reject(&:valid?)

return if !invalid_profiles
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Since invalid_profiles will always be an array, it will always be truthy so this guard statement isn't doing much atm. Maybe Rails helpers like .present? or .blank? would be useful here?

Not a big deal though because if invalid_profiles is an empty array the block below won't execute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the nit!

class CleanupInvalidPartnerProfiles < ActiveRecord::Migration[7.1]
def up
# ActiveRecord::Base.logger = Logger.new(STDOUT)
invalid_profiles = Partners::Profile.all.reject(&:valid?)
Copy link
Collaborator

@coalest coalest Dec 5, 2024

Choose a reason for hiding this comment

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

One other question. Are there many Partners::Profile records in production? If there are many, then loading them all at once like this could cause a memory spike and fire off thousands of database queries.

The safer option when dealing with lots of records would be using something like find_each:

Partners::Profile.find_each(batch_size: SOME_NUMBER) do |profile|
  next if profile.valid?
  # code
  sleep(0.01) # throttle
end

If there aren't many records it doesn't really matter and ignore this comment.

Copy link
Collaborator Author

@cielf cielf Dec 5, 2024

Choose a reason for hiding this comment

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

Define many g. There are about 6000.
I have been testing this against prod data, and it will run during a maintenance window,

Copy link
Collaborator

Choose a reason for hiding this comment

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

find_each is technically better, but for a one-off migration like this it's usually not worth the fuss.

Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

Looks good to me! I trust you with the data testing. :)

@dorner dorner merged commit 5cce3c1 into rubyforgood:main Dec 10, 2024
12 checks passed
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.

3 participants