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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable the Django Rest Framework built-in documentation feature #1832

Merged
merged 4 commits into from
Jul 9, 2019

Conversation

reupen
Copy link
Contributor

@reupen reupen commented Jul 5, 2019

Description of change

This enables the Django Rest Framework built-in documentation feature at the URL path /docs.

This is currently only enabled if the ENABLE_API_DOCUMENTATION environment variable is set to True as the documentation is not fully functional as yet.

Currently, a user must log into Django admin prior to accessing the documentation.

Some minor changes had to be made to a few views in order to get the view introspection working. (In the case of the D&B views, they were not really generic views and so have been changed to use APIView instead of GenericAPIView.)

The next version of DRF moves more towards OpenAPI-based schema generation and these changes will help us in switching to that when we upgrade.

I'm planning to fix a few things in further PRs, and then will probably enable the docs by default for (at least) local and the dev environments. Will also add some docs to make it clear what needs to be done for new endpoints.

Screenshots

These show the docs functionality in DRF 3.9 (this may change when we upgrade to 3.10).

image

image

Checklist

  • Has a new newsfragment been created? Check changelog/README.rst for instructions
  • Have any relevant search models been updated?
  • Have any relevant fixtures (fixtures/test_data.yaml) been updated?
  • Have any relevant select-/prefetch-related field lists in the views and search apps been updated?
  • Has the admin site been updated (for new models, fields etc.)?
  • Has the README been updated (if needed)?

@reupen reupen requested review from currycoder, alixedi and elcct July 5, 2019 15:34
@reupen reupen removed request for currycoder, alixedi and elcct July 8, 2019 09:14
@reupen reupen force-pushed the feature/api-docs branch 2 times, most recently from bc9baf8 to 2551ca8 Compare July 8, 2019 12:41
@codecov
Copy link

codecov bot commented Jul 8, 2019

Codecov Report

Merging #1832 into develop will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1832      +/-   ##
===========================================
+ Coverage    97.24%   97.24%   +<.01%     
===========================================
  Files          308      308              
  Lines         9616     9629      +13     
  Branches       981      982       +1     
===========================================
+ Hits          9351     9364      +13     
  Misses         181      181              
  Partials        84       84
Impacted Files Coverage Δ
datahub/investment/project/views.py 96.19% <ø> (ø) ⬆️
datahub/investment/project/proposition/views.py 95.23% <ø> (ø) ⬆️
datahub/omis/invoice/views.py 100% <100%> (ø) ⬆️
datahub/omis/order/views.py 100% <100%> (ø) ⬆️
datahub/omis/quote/views.py 100% <100%> (ø) ⬆️
datahub/dnb_match/views.py 98.48% <100%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78904d3...cc88f43. Read the comment docs.

Copy link
Contributor

@currycoder currycoder left a comment

Choose a reason for hiding this comment

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

Looks great to me - one minor comment inline.

Also a question - with the OpenAPI changes proposed, will the swagger definition go as far as explaining field validation criteria..?

datahub/omis/invoice/views.py Show resolved Hide resolved
@reupen
Copy link
Contributor Author

reupen commented Jul 8, 2019

Also a question - with the OpenAPI changes proposed, will the swagger definition go as far as explaining field validation criteria..?

I expect not, but it should hopefully still be possible to add a description (with general comments) for each view.

You can have a look at it looks like in DRF 3.10 via the docs on their master branch:

They seem to be in the process of releasing DRF 3.10.0 right now so it shouldn't be far off at all (but we will need an update to django-filter as well for carltongibson/django-filter#1086).

Copy link
Contributor

@alixedi alixedi left a comment

Choose a reason for hiding this comment

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

One small observation. Otherwise looks good 👍

"""Base matching information APIView."""

required_scopes = (Scope.internal_front_end,)

queryset = Company.objects.select_related('dnbmatchingresult')

lookup_url_kwarg = 'company_pk'
def _get_company(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please pass down self.kwargs['company_pk'] as an argument to this method? The same for _get_matching_information. I don't think utility functions should concern themselves with the existence of self.kwargs['company_pk'].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change _get_matching_information() to take a company as that one does seem a bit weird.

However, I think _get_company() fits in with how get_object() works in generic views and it would be a bit repetitive if we started writing self.kwargs['company_pk'] everywhere.

reupen added 4 commits July 9, 2019 15:17
This is to make views compatible with DRF schema introspection.
This is because they aren't actually generic views, and inheriting from GenericAPIView breaks DRF schema introspection.
This refactors the views slightly so that `Http404`s are not raised when building serializer contexts if the order referenced in the URL path does not exist (which breaks DRF schema introspection).

Instead, this is checked as part of the request dispatch process (which seems like a more logical place to do this kind of check).
This enables the Django Rest Framework built-in documentation feature at the URL path `/docs`.

This is currently only enabled if the `ENABLE_API_DOCUMENTATION` environment variable is set to `True` as the documentation is not fully functional as yet.

A user must log into Django admin prior to accessing the documentation.

The DRF 3.9 will move more to OpenAPI-based schema generation and these changes will help us to start using that when we upgrade.
@reupen reupen force-pushed the feature/api-docs branch from 71e466c to cc88f43 Compare July 9, 2019 14:56
@reupen reupen merged commit b9bc3a0 into develop Jul 9, 2019
@reupen reupen deleted the feature/api-docs branch July 9, 2019 15:06
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.

3 participants