-
Notifications
You must be signed in to change notification settings - Fork 10
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
🐛[#449] support objecttypes API with pagination #454
Conversation
v2_service = ServiceFactory( | ||
api_root=self.object_types_api.format(version="v2"), | ||
auth_type=AuthTypes.api_key, | ||
header_key="Authorization", | ||
header_value="Token 5cebbb33ffa725b6ed5e9e98300061218ba98d71", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should a docker container be made for objecttypes for testing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. We should be able to change and modify these tests.
If you decide to use vcr let's make sure we can reuse it:
- please add docker-compose with Objecttypes
- add fixture for Obejcttypes if you use it
- add README which describes how to use it if you want to update cassettes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've split this into a separate issue #457
caea548
to
bca880e
Compare
Idk why this is failing, I'll look into it later edit: fixed |
bca880e
to
c465f53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea to using vcr for Objecttypes API, but I think it needs a little more love
v2_service = ServiceFactory( | ||
api_root=self.object_types_api.format(version="v2"), | ||
auth_type=AuthTypes.api_key, | ||
header_key="Authorization", | ||
header_value="Token 5cebbb33ffa725b6ed5e9e98300061218ba98d71", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. We should be able to change and modify these tests.
If you decide to use vcr let's make sure we can reuse it:
- please add docker-compose with Objecttypes
- add fixture for Obejcttypes if you use it
- add README which describes how to use it if you want to update cassettes
3e1098e
to
6baedab
Compare
@@ -52,6 +52,10 @@ def get_data_field_choices(self): | |||
except requests.JSONDecodeError: | |||
continue | |||
|
|||
# TODO: remove check once API V1 is removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check will be pointless once Objectstypes API V1 is discontinued
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #454 +/- ##
==========================================
+ Coverage 95.18% 95.31% +0.13%
==========================================
Files 150 155 +5
Lines 5105 5485 +380
==========================================
+ Hits 4859 5228 +369
- Misses 246 257 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
form = response.context["adminform"].form | ||
choices = list(form.fields["object_type"].choices) | ||
|
||
self.assertEqual( | ||
choices[1][0].value, | ||
object_type.id, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there was an easier way to check this with normal Django but maybe I'm thinking of webtest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm yeah I think that's probably webtest
form = response.context["adminform"].form | ||
choices = list(form.fields["object_type"].choices) | ||
|
||
self.assertEqual( | ||
choices[1][0].value, | ||
object_type.id, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm yeah I think that's probably webtest
|
||
self.assertEqual( | ||
choices[1][0].value, | ||
object_type.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bit of a nitpick, but is it possible to check the label here as well? Currently it will probably just check if the value
is 1
, which doesn't really say a lot 😬
6baedab
to
36f9c6f
Compare
Fixes #449
Supports paginated response from OT V2