Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

Phone field fixes #853

Merged
merged 5 commits into from Feb 1, 2017
Merged

Phone field fixes #853

merged 5 commits into from Feb 1, 2017

Conversation

NealJMD
Copy link
Contributor

@NealJMD NealJMD commented Jan 27, 2017

@edubsky has been working with phone numbers and turned up two important bugs that I worked on today -

  • Our member record was only updated with the fields that we'd defined as columns in the member table. That didn't include phone, so it meant that any form that had phone would never have that field prefilled. Rather than just add another column to the table, I added a catchall jsonb column that stores any AK-valid field so this never happens again.
  • Our QueuePusher pushed only a subset of valid user fields to AK for donations (petitions were doing fine) so phone numbers were not getting pushed to AK.

These are both fixed. After we deploy, I'm inclined to run something like this to bring the current member records up to speed -

    Members.all.include(:actions).each do |m|
      params = m.actions.map(&:form_data).reduce({}, :merge)
      MemberUpdater.run(m, params)
    end

hash = @params.try(:to_unsafe_hash) || @params.to_h # for ActionController::Params
hash.symbolize_keys.compact.keep_if { |k| permitted_keys.include? k }
def member_attributes
@member_attributes ||= Member.new.attributes.keys.map(&:to_sym).select { |k| k != :id }
Copy link
Member

Choose a reason for hiding this comment

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

You can call column_names on the class:

Member.column_names.map(&:to_sym)[1..-1]

@@ -67,6 +67,10 @@ class ActionKitFields < ActiveModel::Validator
action_
).freeze

def initialize(name)
@name = name
Copy link
Member

Choose a reason for hiding this comment

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

If you're worried name isn't a string, then perhaps call to_s on it here, rather than below? https://github.com/SumOfUs/Champaign/pull/853/files#diff-e52e089e43530d3d8f597e45e9128cfcR106

end

def has_valid_prefix
@name =~ VALID_PREFIX_RE
(@name =~ VALID_PREFIX_RE).present?
Copy link
Member

Choose a reason for hiding this comment

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

Is present? necessary? nil is falsy after all. If a boolean is a must then !(@name !~ VALID_PREFIX_RE) is more expressive, in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm taking your other suggestions, but not this one - !(@name !~ VALID_PREFIX_RE) is super arcane to me, as I've never seen !~ before and it never occurs in our codebase, whereas =~ is very common. I also think that methods beginning in is_ or has_ should return booleans.

@osahyoun
Copy link
Member

A very fine PR 💯. Merge with extreme prejudice, but not before responding to my comments!

@osahyoun
Copy link
Member

osahyoun commented Jan 31, 2017

Your suggested data migration above would bring the site to its knees. Think about using find_each instead of all

http://apidock.com/rails/ActiveRecord/Batches/find_each

@NealJMD NealJMD merged commit cbec1f3 into development Feb 1, 2017
@NealJMD NealJMD deleted the phone-fixes branch February 1, 2017 02:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants