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

Add V3 custom fields endpoints #60

Merged
merged 1 commit into from
May 29, 2020

Conversation

timchaston
Copy link
Contributor

@timchaston timchaston commented May 25, 2020

Hi,

Wrote up the endpoints for contact custom fields. I've also done some work on endpoints for custom field values - which is primarily what I'm needing to get working - but I realised that it would make sense to submit this first as groundwork.

Let me know what adjustments are needed.

Thanks,
Tim

@timchaston timchaston force-pushed the v3-custom-fields branch 4 times, most recently from f07e9e8 to c1a2ea7 Compare May 25, 2020 09:48
@timchaston
Copy link
Contributor Author

timchaston commented May 26, 2020

I see the CI is failing, seemingly because the environment variables aren't set, due to Travis CI security restrictions. Are you able to tell me if there's anything I need to do from my side to address that?

Copy link
Owner

@mhenrixon mhenrixon left a comment

Choose a reason for hiding this comment

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

Only one small remark where you think you are testing the update but in fact the field was already created with those values so the update isn't really testing anything.

Unfortunately, I don't know of a way of sharing the tokens. The one reason I don't want to have my own test account available to the public is to avoid having to clean up data from failed test runs so I suggest on your travis account that you add the missing environment variables for

  • ACTIVE_CAMPAIGN_URL
  • ACTIVE_CAMPAIGN_TOKEN

At least if you plan on creating another PR it would make sense to do that to make it easier to review if the code is tested properly and tests are passing.

Normally (for other services), it wouldn't be a problem but I've had quite a few issues with testing ActiveCampaign.

spec/lib/active_campaign/api/fields_spec.rb Outdated Show resolved Hide resolved
lib/active_campaign/api/fields.rb Show resolved Hide resolved
spec/support/shared_contexts/with_field.rb Outdated Show resolved Hide resolved
@mhenrixon
Copy link
Owner

Actually since I believe I filter the tokens it might be enough to set something dummy unless the environment variable is configured

I’ll have a look at that

@mhenrixon
Copy link
Owner

It should build properly if you merge my master into your branch now! 👍

Copy link
Owner

@mhenrixon mhenrixon left a comment

Choose a reason for hiding this comment

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

Ok, I try to explain myself a little better. Basically, for any single test. All HTTP interactions are recorded into a single file. That is why the name of it "returns a field_hash is so darn important.

A couple of things that I would like to improve (to avoid having to be so picky):

  1. Reusable VCR cassettes (meaning I can select the cassettes at will as requests are made. Like when the user or contact is created I pick a cassette for that specific request to avoid this duplication in the test setup. The problem is that I constantly discover dependencies between contacts, pipelines, etc having. to be recorded in a specific order that I haven't figured out the best way to do this yet. If you have any suggestions I'm all ears. This way we could name the cassettes based on resources instead of tests which would help avoid the confusion like between you and me right now. Another possibility would be to better match the cassettes on interaction and let VCR pick one for another test. Don't know if the last one is an option though.
  2. I'd like to then remove the strings in the tests so that the entire test line becomes it { is_expected.to eq(expected_result) } because I find the maintenance of test strings to be cumbersome and error prone but until I fix number 1 this leads to really confusing cassette names so for the sake of clarity I'd like to be as specific as possible about what type of hash is expected in the test name.

spec/lib/active_campaign/api/fields_spec.rb Outdated Show resolved Hide resolved
spec/lib/active_campaign/api/fields_spec.rb Outdated Show resolved Hide resolved
spec/lib/active_campaign/api/fields_spec.rb Outdated Show resolved Hide resolved
spec/lib/active_campaign/api/fields_spec.rb Outdated Show resolved Hide resolved
@codeclimate
Copy link

codeclimate bot commented May 29, 2020

Code Climate has analyzed commit 73b59bf and detected 0 issues on this pull request.

View more on Code Climate.

@timchaston
Copy link
Contributor Author

I've updated the PR in line with your feedback. CI is looking good!

Copy link
Owner

@mhenrixon mhenrixon left a comment

Choose a reason for hiding this comment

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

Nice work and many thanks for your contribution.

@mhenrixon mhenrixon merged commit e11aa5b into mhenrixon:master May 29, 2020
@mhenrixon mhenrixon added the v3 label May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants