Fix missing customer_uuid when creating a note #88
Conversation
|
Code Climate has analyzed commit 6d347d7 and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 98.4% (0.0% change). View more on Code Climate. |
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| @@ -191,7 +191,7 @@ def _validate_arguments(cls, method, kwargs): | |||
| ) | |||
|
|
|||
| @classmethod | |||
| def _method(callerClass, method, http_verb, path=None, useCallerClass=False): | |||
| def _method(callerClass, method, http_verb, path=None, useCallerClass=False, useUUIDFor=None): | |||
There was a problem hiding this comment.
Function _method has 5 arguments (exceeds 4 allowed). Consider refactoring.
There was a problem hiding this comment.
I want to improve this line of code. If there is a better way, please feel free to comment on it
There was a problem hiding this comment.
@SoeunSona The useUUIDFor being used only for this endpoint seems like a code smell to me. There should be a better way to do this. And perhaps doing what I suggested before (https://github.com/chartmogul/platform/pull/19166#issuecomment-1853150940), regarding supporting both /customers/:customer_uuid/notes and /customer_notes for both listing all notes and creating a note should help this.
The end result would be something like this:
Customer.notes = CustomerNote._method("all", "get", "/customers/{uuid}/notes", useCallerClass=True)
Customer.createNote = CustomerNote._method("create", "post", "/customers/{uuid}/notes", useCallerClass=True)Just like how I did it for contacts:
chartmogul-python/chartmogul/api/customer.py
Lines 82 to 85 in ed81ce8
This means we have to modify the core API code to support both endpoints as such. What do you think?
There was a problem hiding this comment.
@briwa Thank you for suggesting this idea. But It seems overkill because we need to change not only the core api but also change all the libraries. And it also requires communication cost with product team to update. I think it would be better to release without this
There was a problem hiding this comment.
I agree that this isn't great, and @briwa solution is a good one, but I think this is a compromise we can live with, specially because even if we then change the underlying API we can keep this around for backwards compatibility.
|
Just FYI I will be off and back on Dec 26th. Feel free to find other reviewers if the PR needs to be merged before then, but I will check this PR again once I'm back |
No description provided.