search

jpetrucciani / hubspot3

python3.6+ hubspot client based on hapipy, but modified to use the newer endpoints and non-legacy python
MIT License
147 stars 73 forks source link

added support for custom ticket properties #74

Closed huntdesignco closed 4 years ago

huntdesignco commented 4 years ago

While developing a ticketing system using this API, I found that support for custom ticket properties was missing. I modified the necessary files to allow for the following:

tickets = client.tickets.get_all(properties=["subject", "content", "hs_pipeline", "hs_pipeline_stage", "created_by", "hs_ticket_category", "hubspot_owner_id"])

as well as:

ticket = client.tickets.get(10239456, properties=["subject", "content", "hs_pipeline", "hs_pipeline_stage", "created_by", "hs_ticket_category", "hubspot_owner_id"])

I found it extremely valuable being able to specify custom properties. The implementation of this feature may or may not be the proper approach, as I am new to Python. Nonetheless, I find myself having to keep merging my changes locally every time I update my hubspot3 API code.

jpetrucciani commented 4 years ago

Thanks for your contribution! I should be able to review this later today

huntdesignco commented 4 years ago

Hi,

Thanks for the feedback and I appreciate letting me contribute. Please see below:

In setup.cfg:

@@ -1,9 +1,14 @@ [mypy] -python_version = 3.5 +python_version = 3.6 For now we're still supporting python 3.5, so we'd need to keep this as is

^^ This is fine. I am using Python 3.6 for my application which is why it was changed.

In hubspot3/tickets.py:

     )

Normally, we'd do something more like this:

def get_all(self, limit: int = -1, properties: List[str] = None, **options) -> list: properties = properties or ["subject", "content", "hs_pipeline", "hs_pipeline_stage", "hs_pipeline"]

I a fine with this change and it makes more sense. I also agree with changing the following to match the above code:

def get(self, ticket_id: str, properties: List[str] = None, include_deleted: bool = False, **options) -> Dict: properties = properties or ["subject", "content", "hs_pipeline", "hs_pipeline_stage", "hs_pipeline"]

I think this code is fine as is. It appears to function without any issues

In hubspot3/base.py:

@@ -110,9 +110,10 @@ def _prepare_request_auth(self, subpath, params, data, opts): params["hapikey"] = params.get("hapikey") or self.api_key

  def _prepare_request(

This should be a list ( [] ) self, subpath, params, data, opts, doseq=False, query="", retried=False, properties=[]

In hubspot3/base.py:

 ):

params = params or {}

  • properties = properties or {} Unless this needs to be a dict (or set?), it may be better as a list:

properties = properties or []

^ this is also correct

In hubspot3/base.py:

@@ -139,6 +140,11 @@ def _prepare_request( if data and headers["Content-Type"] == "application/json" and not retried: data = json.dumps(data)

for property in properties: url += '&properties={}'.format(property)

I also normally prefer .format() to string concatenation, but it works either way.

I agree with this change as well.

In hubspot3/base.py:

@@ -343,6 +350,7 @@ def _call( doseq: bool = False, query: str = "", raw: bool = False,

  • properties: str ="", String or list?

Should be a list:

     properties: List[str] = None,

( I assume the above would be correct )

In setup.cfg:

disallow_untyped_defs = False ignore_missing_imports = True

[flake8] ignore = N802,N807,W503 max-line-length = 100 max-complexity = 20 + +[egg_info] +tag_build = +tag_date = 0 + Normally I try not to add things here unless explicitly necessary

I'm not sure why this was added or changed as it should not have been, possibly happened when I rebundled the package in .tar.gz format using python3.6

Can be reverted/omitted

In hubspot3/globals.py:

@@ -1,7 +1,7 @@ """ globals file for hubspot3 """ -version = "3.2.36" +version = "3.2.37" Normally I'll handle updating this version number, as I need to manually trigger a new release to pip

Sounds good. I wasn't sure how this should be handled either. I set up a private python repo and was just using that repo to install from, rather than the public repo.

I appreciate the feedback and I can certainly get my code updated and re-submit the pull request after testing the new code. A lot of these changes were made during my first week of learning Python. I was thrown into a project without any prior knowledge of Python (I am a PHP developer), so I figured the code could be written better and I haven't looked at it since my Python knoweldge has gotten better.

I look forward to continuing to help with contributing to the API as it has been critical in the project I recently developed.

Let me know if there is a particular route I should take to get the new changes added. I assume updating my code and re-submitting the pull request is the proper route to take. ( I am new to contributing to projects with git ).

Best Regards, Will

On 2019-11-14 19:32, Jacobi Petrucciani wrote:

@JPETRUCCIANI requested changes on this pull request.

Left a few comments here on various pieces - I like the idea, and I'd be happy to merge once we've worked out a few things!

Also, the GitHub action checks [1] are there to help out a bit, so make sure to check those! They should be run on every commit to the branch once you've opened a PR. I currently use black [2] to auto-format the code to keep style consistent (which is one of the checks), as well as prospector [3] for general static analysis to catch other issues. Mypy [4] is also in the checks, but isn't too strict since I haven't added type annotations to everything yet

Let me know if this all makes sense - I tried to explain a bit more since you said you're a bit newer to python 😄

In setup.cfg [5]:

@@ -1,9 +1,14 @@ [mypy] -python_version = 3.5 +python_version = 3.6

For now we're still supporting python 3.5, so we'd need to keep this as is

In hubspot3/tickets.py [6]:

     )
  • def get_all(self, limit: int = -1, **options) -> list:
  • def get_all(self, properties=["subject", "content", "hs_pipeline", "hs_pipeline_stage", "hs_pipeline"], limit: int = -1, **options) -> list:

This line could be dangerous - using a mutable value as a default for a kwarg can lead to unintentional behavior [7].

Normally, we'd do something more like this:

def get_all(self, limit: int = -1, properties: List[str] = None, **options) -> list:

some code

properties = properties or ["subject", "content", "hs_pipeline",

"hs_pipeline_stage", "hs_pipeline"]

In hubspot3/tickets.py [8]:

@@ -42,7 +42,12 @@ def create( ticket_data.append({"name": "hs_pipeline_stage", "value": stage}) return self._call("objects/tickets", data=ticket_data, method="POST", **options)

  • def get(self, ticket_id: str, include_deleted: bool = False, **options) -> Dict:
  • def update(self, ticket_id: str, data: dict, **options) -> Dict:
  • return self._call(
  • "objects/tickets/{}".format(ticket_id), method="PUT", data=data, **options
  • )
  • def get(self, ticket_id: str, properties=["subject", "content", "hs_pipeline", "hs_pipeline_stage", "hs_pipeline"], include_deleted: bool = False, **options) -> Dict:

See the explanation below

In hubspot3/base.py [9]:

@@ -110,9 +110,10 @@ def _prepare_request_auth(self, subpath, params, data, opts): params["hapikey"] = params.get("hapikey") or self.api_key

 def _prepare_request(
  • self, subpath, params, data, opts, doseq=False, query="", retried=False
  • self, subpath, params, data, opts, doseq=False, query="", retried=False, properties=""

Is this to be passed as a string or as a list?

In hubspot3/base.py [10]:

 ):

params = params or {}

  • properties = properties or {}

Unless this needs to be a dict (or set?), it may be better as a list:

properties = properties or []

In hubspot3/base.py [11]:

@@ -139,6 +140,11 @@ def _prepare_request( if data and headers["Content-Type"] == "application/json" and not retried: data = json.dumps(data)

  • i = 0
  • while i < len(properties):
  • url = url + "&properties=" + properties[i]
  • i += 1

If we're using a list here after making the above changes, it's generally a bit more readable/pythonic to use a for loop in this case (also no need for a counter 😉):

for property in properties: url += '&properties={}'.format(property)

I also normally prefer .format() to string concatenation, but it works either way.

In hubspot3/base.py [12]:

@@ -343,6 +350,7 @@ def _call( doseq: bool = False, query: str = "", raw: bool = False,

  • properties: str ="",

String or list?

In setup.cfg [13]:

disallow_untyped_defs = False ignore_missing_imports = True

[flake8] ignore = N802,N807,W503 max-line-length = 100 max-complexity = 20 + +[egg_info] +tag_build = +tag_date = 0 +

Normally I try not to add things here unless explicitly necessary

In hubspot3/globals.py [14]:

@@ -1,7 +1,7 @@ """ globals file for hubspot3 """ -version = "3.2.36" +version = "3.2.37"

Normally I'll handle updating this version number, as I need to manually trigger a new release to pip

-- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [15], or unsubscribe [16].

Links:

[1] https://github.com/jpetrucciani/hubspot3/pull/74/checks?check_run_id=303136089 [2] https://github.com/psf/black [3] https://github.com/PyCQA/prospector [4] https://github.com/python/mypy [5] https://github.com/jpetrucciani/hubspot3/pull/74#discussion_r346608604 [6] https://github.com/jpetrucciani/hubspot3/pull/74#discussion_r346609624 [7] https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html [8] https://github.com/jpetrucciani/hubspot3/pull/74#discussion_r346609835 [9] https://github.com/jpetrucciani/hubspot3/pull/74#discussion_r346610044 [10] https://github.com/jpetrucciani/hubspot3/pull/74#discussion_r346610238 [11] https://github.com/jpetrucciani/hubspot3/pull/74#discussion_r346611182 [12] https://github.com/jpetrucciani/hubspot3/pull/74#discussion_r346611247 [13] https://github.com/jpetrucciani/hubspot3/pull/74#discussion_r346612509 [14] https://github.com/jpetrucciani/hubspot3/pull/74#discussion_r346612711 [15] https://github.com/jpetrucciani/hubspot3/pull/74?email_source=notifications&amp;email_token=ANUWQ5FVTMHTR4H5MJMSXLLQTXU3NA5CNFSM4JNNEJA2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLVDZQI#pullrequestreview-317340865 [16] https://github.com/notifications/unsubscribe-auth/ANUWQ5GSK6VPFEX5CDZQTYLQTXU3NANCNFSM4JNNEJAQ

jpetrucciani commented 4 years ago

Absolutely! Welcome to the python community - we're glad to have you!

Also, I'm glad you've found this repo useful - it makes me happy to know that!

As for the changes, you should just be able to make them locally and push them to your particular fork of this repo, and then the tests/PR will be updated to match the latest commit of your fork

huntdesignco commented 4 years ago

Good morning,

Thanks for the welcoming! I updated my code but the following did not work :

List[str] = None

rather than using this, I just used:

list() = None

I updated the code on my live project and it seems to be working fine with most of the updates you requested. I pushed the updates to my forked repo and will check the pull status.

Best Regards, Will

On 2019-11-14 20:30, Jacobi Petrucciani wrote:

Absolutely! Welcome to the python community - we're glad to have you!

Also, I'm glad you've found this repo useful - it makes me happy to know that!

As for the changes, you should just be able to make them locally and push them to your particular fork of this repo, and then the tests/PR will be updated to match the latest commit of your fork

-- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2].

Links:

[1] https://github.com/jpetrucciani/hubspot3/pull/74?email_source=notifications&amp;email_token=ANUWQ5C66BAV2JNVHW6XEF3QTX3VFA5CNFSM4JNNEJA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEED655I#issuecomment-554168053 [2] https://github.com/notifications/unsubscribe-auth/ANUWQ5ETWKSW3P6W5UOGXQLQTX3VFANCNFSM4JNNEJAQ

huntdesignco commented 4 years ago

The changes have been made. I am not sure what to do next to have the pull request re-tested/submitted.

huntdesignco commented 4 years ago

After a handful of fixes and commits, everything seems okay. In my research, the black test failing is normal. But all other tests seem to pass. Syntax is proper and the code is working. Please let me know if you need anything else changed.

jpetrucciani commented 4 years ago

Looks good to me!

Thank you for the quick fixes - I can fix up the black issues after merging. Hopefully I can get a release with this out soon!

jpetrucciani commented 4 years ago

This should now be live on pip as version 3.2.37!

Thanks again for your contribution! 😄

huntdesignco commented 4 years ago

Good morning,

You're welcome and thank you for making the additions. I am grateful. Now I do not have to manage my own custom hubspot3 package for some of my projects. I will continue to contribute as I add more. If you need any extra help or have any ideas, please let me know as I would be happy to assist with extending the code base.

My direct e-mail is will@huntdesignco.com . Feel free to e-mail me anytime if you want some extra help.

Have a great day!

Best Regards, Will

On 2019-11-15 18:35, Jacobi Petrucciani wrote:

This should now be live on pip as version 3.2.37 [1]!

Thanks again for your contribution! 😄

-- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [2], or unsubscribe [3].

Links:

[1] https://github.com/jpetrucciani/hubspot3/releases/tag/3.2.37 [2] https://github.com/jpetrucciani/hubspot3/pull/74?email_source=notifications&amp;email_token=ANUWQ5BPKQQKKVHPB74A3ZLQT4W3RA5CNFSM4JNNEJA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEHBFQQ#issuecomment-554570434 [3] https://github.com/notifications/unsubscribe-auth/ANUWQ5A44AJDT4LUGKA7ST3QT4W3RANCNFSM4JNNEJAQ

jpetrucciani commented 4 years ago

That's awesome! I'm glad that this will save you some time and maintenance

Thank you for that offer as well - any and all contributions/ideas are welcome!

Have a great day 😄