Skip to content

Add webhooks resource - #15

Merged
gabelimon merged 5 commits into
bigcommerce:masterfrom
Alir3z4:add-webhooks-resource
Jan 28, 2015
Merged

gabelimon merged 5 commits into
bigcommerce:masterfrom
Alir3z4:add-webhooks-resource

Conversation

@Alir3z4

@Alir3z4 Alir3z4 commented Jul 19, 2014

Copy link
Copy Markdown
Contributor

@Alir3z4

Alir3z4 commented Aug 16, 2014

Copy link
Copy Markdown
Contributor Author

Would be great to know when this will be into master or at least can someone review this code and let me know if there's any further development required on it?

@tgsergeant

Copy link
Copy Markdown
Contributor

Tentative 👍 from me. My only suggestion is to squash down the commits before they go into master.

As for getting this merged, maybe @maetl can help?

@maetl

maetl commented Sep 5, 2014

Copy link
Copy Markdown
Contributor

@tgsergeant not me, as I no longer have merge access to any of these public repos.

@Alir3z4

Alir3z4 commented Sep 20, 2014

Copy link
Copy Markdown
Contributor Author

You mean the repo doesn't have a maintainer at the moment ?

@hockeybuggy

Copy link
Copy Markdown
Contributor

So no maintainer?

@hockeybuggy

Copy link
Copy Markdown
Contributor

There is a bit of a quirk in this PR. A DeleteableApiResource has the delete_all method but the Big Commerce API does not have a corresponding endpoint:

DELETE https://api.bigcommerce.com/stores/(store_id)/v2/hooks
(Results in a 404)

This PR is still quite usable.

@Alir3z4

Alir3z4 commented Nov 27, 2014

Copy link
Copy Markdown
Contributor Author

@hockeybuggy I use this patch in my application and I haven't tried delete all on the resource but for sure I can delete webhook objects:

https://developer.bigcommerce.com/api/stores/v2/webhooks#delete-hook

@hpk

hpk commented Dec 2, 2014

Copy link
Copy Markdown

+1 on this. Would love to see this merged.

@gabelimon

Copy link
Copy Markdown
Contributor

Since webhooks doesn't allow delete_all we'll need to re-work this a bit. The solutions that come to mind are

  1. Adding a SingleDeleteableResource (I'm open to names) which DeleteableResource inherits from and which the webhooks resource uses instead of DeleteableResource
  2. Having the webhooks resource throw an exception if delete_all is called

I'll give this some thought but please chime in if you guys feel strongly one way or the other or if you have a less ugly solution.

@hockeybuggy hockeybuggy mentioned this pull request Jan 22, 2015
@hockeybuggy

Copy link
Copy Markdown
Contributor

I implemented the second option proposed by @gabelimon here -> Alir3z4/pull/1

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.

Also, would you mind removing the extra line?

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.

I could but, I think it would be a style violation. When I remove it and run pep8 against the file:

➤ pep8 bigcommerce/resources/webhooks.py
bigcommerce/resources/webhooks.py:3:1: E302 expected 2 blank lines, found 1

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.

Cool, then leave it.

@hockeybuggy

Copy link
Copy Markdown
Contributor

@gabelimon So this is ok to merge into master after @Alir3z4 gets a chance to merge Alir3z4#1 ?

@gabelimon

Copy link
Copy Markdown
Contributor

Yeah we're good to go if he merges your change.

…delete-all

Overrode the delete_all method on Webhooks.

Thanks @hockeybuggy
@Alir3z4

Alir3z4 commented Jan 28, 2015

Copy link
Copy Markdown
Contributor Author

@gabelimon @hockeybuggy I merged the pull request ;)

@gabelimon

Copy link
Copy Markdown
Contributor

👍
We're good to go. Next time please squash your commits though.

gabelimon added a commit that referenced this pull request Jan 28, 2015
@gabelimon
gabelimon merged commit 2cd6083 into bigcommerce:master Jan 28, 2015
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.

6 participants