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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

2022 refresh #6

Merged
merged 9 commits into from Jun 29, 2022
Merged

2022 refresh #6

merged 9 commits into from Jun 29, 2022

Conversation

Intrepidd
Copy link
Collaborator

馃憢

Trying to resurrect this lib

  • Testing against modern ruby & activesupport versions
  • Re-recorded all tests, that was a challenge as now we need to test again a real hubspot portal, before we could use the demo api key for POST / PUT requests but that's not allowed anymore, also some ids were hardcoded in the specs, that was fun 馃槗
  • Re-wrote the association API as it was using API v1 that has bugs (dissociate does not work for instance), so I had to slightly change the API

Copy link

@Paul-Yves Paul-Yves left a comment

Choose a reason for hiding this comment

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

Congrats for the hard work. Just a little nitpicking

lib/hubspot/contact_list.rb Show resolved Hide resolved
Copy link

@fonji fonji left a comment

Choose a reason for hiding this comment

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

Bon je pinaille un peu, mais en soit, y a que la constante 脿 nettoyer qui me freine 脿 envoyer 莽a.
C'est grave si je commente les PRs en fran莽ais ?

Comment on lines 69 to 75
if response.success?
response.parsed_response
response
elsif response.not_found?
raise(Hubspot::NotFoundError.new(response))
else
raise(Hubspot::RequestError.new(response))
end
Copy link

Choose a reason for hiding this comment

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

github ne me laisse pas faire une suggestion mais bon, on s'la fait guard clause ?

return response if response.success?

response.not_found? ? raise(Hubspot::NotFoundError.new(response)) : raise(Hubspot::RequestError.new(response))

alternative 1

return response if response.success?

error_klass = response.not_found? ? Hubspot::NotFoundError : Hubspot::RequestError
raise error_klass.new(response)

alternative 2

return response if response.success?

raise(Hubspot::NotFoundError.new(response)) if response.not_found? 
raise(Hubspot::RequestError.new(response))

@@ -92,12 +92,6 @@ def contacts(opts={})
end
end

# {http://developers.hubspot.com/docs/methods/lists/refresh_list}
def refresh
response = Hubspot::Connection.post_json(REFRESH_PATH, params: { list_id: @id, no_parse: true }, body: {})
Copy link

Choose a reason for hiding this comment

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

faut virer la constante non ?
Pourquoi on d茅gage 莽a en fait ? 馃 Bon je trouve pas de doc j'imagine que 莽a n'existe pas.

Copy link

Choose a reason for hiding this comment

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

(tiens 莽a fait r茅p茅tition avec le commentaire de Paul-Yves 馃槄 )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oui l'endpoint n'existe plus 脿 priori ...

Comment on lines +213 to +215
expect(list.id).to be == list1.id
expect(lists.second.id).to be == list2.id
expect(lists.last.id).to be == list3.id
Copy link

Choose a reason for hiding this comment

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

un risque qu'un jour ce ne soit pas le m锚me ordre ?

Suggested change
expect(list.id).to be == list1.id
expect(lists.second.id).to be == list2.id
expect(lists.last.id).to be == list3.id
expect(lists.map(&:id)).to contain_exactly(list1.id, list2.id, list3.id)

@Intrepidd Intrepidd merged commit 59f6915 into master Jun 29, 2022
@Intrepidd Intrepidd deleted the 2022-refresh branch June 29, 2022 08:31
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.

None yet

3 participants