Skip to content

Add support method_name for amo_property option#47

Merged
palkan merged 4 commits intoteachbase:masterfrom
ZloyPotroh:master
Nov 25, 2019
Merged

Add support method_name for amo_property option#47
palkan merged 4 commits intoteachbase:masterfrom
ZloyPotroh:master

Conversation

@ZloyPotroh
Copy link
Copy Markdown
Contributor

@ZloyPotroh ZloyPotroh commented Nov 21, 2019

Due to issue: #39
and thanks to: https://github.com/foxford/amorail/commit/3404da937661230ccc7c9f23f138cdacd65dc477

This will help to do things this way:

class MyAmoLead < Amorail::Lead
  amo_property 'REFERER', method_name: :referer
  amo_property 'from_mark', method_name: :mark
end

Warning: property in code above is in uppercase. To make this works i added downcase:

# lib/amorail/entity/params.rb:25
prop_id = props.send(k.downcase).id

I hope this change does not cause problems. Otherwise getting an error:

NoMethodError: undefined method `REFERER' for #<Amorail::Property::Lead:0x0000555b9dafbdf8>
/app/tmp/amorail/lib/amorail/property.rb:10:in `method_missing'
/app/tmp/amorail/lib/amorail/entity/params.rb:25:in `block in custom_fields'
/app/tmp/amorail/lib/amorail/entity/params.rb:24:in `each'
/app/tmp/amorail/lib/amorail/entity/params.rb:24:in `custom_fields'
/app/tmp/amorail/lib/amorail/entity/params.rb:13:in `params'
/app/tmp/amorail/lib/amorail/entity/params.rb:38:in `create_params'
/app/tmp/amorail/lib/amorail/entity.rb:113:in `push'
/app/tmp/amorail/lib/amorail/entity/persistence.rb:19:in `save'
/app/tmp/amorail/lib/amorail/entity/persistence.rb:23:in `save!'
/app/app/models/lead.rb:31:in `sync_remote'
/app/lib/tasks/amo.rake:15:in `block (2 levels) in <main>'
/bundle/gems/bootsnap-1.4.5/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:54:in `load'
/bundle/gems/bootsnap-1.4.5/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:54:in `load'
-e:1:in `<main>'

Copy link
Copy Markdown
Contributor

@palkan palkan left a comment

Choose a reason for hiding this comment

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

property in code above is in uppercase. To make this works i added downcase:

I think it's better to handle this case in the Property class, 'cause that's where we know
that names are downcased:

hash[identifier.downcase] = PropertyItem.new(contact)

We can add downcase here:

if data.key?(method_sym.to_s)

ZloyPotroh and others added 2 commits November 22, 2019 13:10
Co-Authored-By: Vladimir Dementyev <dementiev.vm@gmail.com>
@ZloyPotroh
Copy link
Copy Markdown
Contributor Author

@palkan Thank you for the quick reaction.
I doubted the correctness of the adding downcase in that place.


def respond_to_missing?(method_sym, *args)
args.size.zero? && data.key?(method_sym.to_s)
args.size.zero? && data.key?(prep_method_name(method_sym))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@palkan Please take a look at this. I understand that is standard case usind respond_to missing?, but I could make mistake because I have little experience in ruby/ror.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, that looks good.
Only one suggestion: let's not extract to_s.downcase to a prep_method_name to avoid adding one more method to the module. It's OK to duplicate here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, done.

@ZloyPotroh ZloyPotroh requested a review from palkan November 22, 2019 11:45
Copy link
Copy Markdown
Contributor

@palkan palkan left a comment

Choose a reason for hiding this comment

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

👍

@palkan palkan merged commit 2a0cd3c into teachbase:master Nov 25, 2019
@palkan
Copy link
Copy Markdown
Contributor

palkan commented Nov 25, 2019

Released in 0.6.1.

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.

2 participants