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

feature/BE-14 #245

Merged
merged 3 commits into from
Nov 9, 2022
Merged

feature/BE-14 #245

merged 3 commits into from
Nov 9, 2022

Conversation

KarahanS
Copy link
Contributor

@KarahanS KarahanS commented Nov 4, 2022

Please review this PR and perform the steps below to get familiar with the concept. There are lots of to learn but I'll try to list down the ones I found useful while documenting the existing APIs.

  • I decided to use drf_yasg module to integrate Swagger to our APIs.
  • Added SWAGGER_SETTINGS to the base.py settings file. This was necessary to test our APIs that require authorization with tokens.
  • Added Swagger URL as a url to the urls.py within the backend folder (not api). After running the application, you can have a look at the Swagger via this url: http://localhost:8000/api/v1/swagger/schema.
  • I removed the following part:
urlpatterns = format_suffix_patterns(urlpatterns, allowed=['json', 'html'])

Each endpoint was displayed twice because of that and I thought it looked too ugly. Let me know if we should've kept that line.

Let me provide my understanding of this module and what features I've used.
First of all, drf_yasg attempts to document all endpoints within the urlpatterns in api/urls.py. Therefor if you add your API there, it will automatically be displayed in Swagger.

However, there are couple of problems. Firstly, it doesn't generate all the possible responses. Secondly, it doesn't provide any description or something. So, we have to provide them manually. To do so, we have to use a decorator called @swagger_auto_schema. This decorator must be called before the relevant function (post, get etc.). At this point, please examine its usage in the repository carefully.

@swagger_auto_schema takes lots of arguments. Here are some of them I used:

  • operation_description: Puts a description about the API
  • operation_summary: Put a summary about the API (this is the one we see at first look)
  • tags: This enables us to group our APIs based on tags (actually I didn't have to use this for now but anyways)
  • request_body: This sets the request body. This part is a little bit complicated and I strongly believe we will learn more about this as time goes by. What I understand though, is that it generates the request body based on the serializer we use. Otherwise, we have to give it manually. (I gave it manually but I didn't have to - I was just trying to get familiar with it)
  • responses: This helps us to list the response types we wait for an API to return. Here we can also provide example outputs - which is very informative for a documentation. So we had better use this frequently.

Additionally, I used something called decorated view for our logout endpoint. This simply lets you to define all things I listed above not before the declaration of the function but while providing it as a view in the urls.py. Why did I use it? Well, because logout endpoint is implemented by Knox and we have to provide some response types to it. So, I believe it was a nice use case for decorated views.
Lastly, there is this button called Authorize on the right corner in Swagger page. We have to use it to test APIs that require authorization. For example, in order to test the logout API, first you have to log in to the application and have a token with you. Try the login endpoint first, copy paste the token returned to you and click on the Authorize button. Input the token in the following format: "Token xxx". Replace xxx with the token and don't forget to put a space between the first word and your token.

That's all, let me know if you have any question in mind or you identify a problem with the current configuration. That's ok if you don't understand all of the above, I can explain in the meeting (if time permits) as well - but try to have a basic idea about what's going on here.

@KarahanS KarahanS added Effort: Medium Priority: Medium This issue should be handled, if there isn't any high priority issue Team: Backend issues related to backend labels Nov 4, 2022
@KarahanS KarahanS requested review from mumcusena and BElifb November 4, 2022 20:55
@KarahanS KarahanS self-assigned this Nov 4, 2022
@KarahanS KarahanS mentioned this pull request Nov 4, 2022
3 tasks
@KarahanS KarahanS added Priority: High This issue must be handled immediately Status: Review Needed A review is needed for this issue labels Nov 6, 2022
This was referenced Nov 6, 2022
@BElifb
Copy link
Contributor

BElifb commented Nov 9, 2022

  • I went through the detailed description (thanks for that btw), I am not a %100 clear on everything (e.g. request_body) but mostly undstood the gist of it.
  • I also pulled the branch to local and tested it out. The documentation site looks really good. I have a few remarks but these don't necessarily mean failure.
    • In the register API, when I tried it out with wrong parameters (e.g. username and password too short), nothing happened when I clicked execute button. I was expecting an error message but it did not execute at all.
    • Also when I gave correct credentials to the register API, it returned 201 (which was an undocumented response). I can share the screenshot if you need. I checked and it wasn't actually documented in the auth.py file, I am a little confused as to if this is the expected behaviour of the API.
  • I also tried out login and logout API's in the described manner (with authentication) and they worked fine.
  • We were using format_suffix_patterns, when making use of Django's JSON rendering, now that we have our own custom frontend I think it is okay to comment it out till we have need.
  • The above issues seem more API related rather than Swagger, so I can at least confirm the documentation functionality is working fine, which is why we decided to use Swagger at the first place.
  • Also for anyone trying out the branch, don't forget to reinstall requirements.txt as there are some new packages.

@BElifb BElifb merged commit 69d31db into master Nov 9, 2022
@KarahanS
Copy link
Contributor Author

KarahanS commented Nov 9, 2022

  • Sometimes execute seems like it gets stuck, I don't understand why that happens - and don't really know if it's due to our code or Swagger itself.
  • Nice catch, thanks. It should return 201 since we create a new user in the database, frontend already takes it as 201 no problem but it doesn't comply with our documentation. I created a hotfix branch for the update, could you review and merge it as well? It should take just a couple of seconds.

@KarahanS KarahanS deleted the feature/BE-14 branch November 9, 2022 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: Medium Priority: High This issue must be handled immediately Priority: Medium This issue should be handled, if there isn't any high priority issue Status: Review Needed A review is needed for this issue Team: Backend issues related to backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants