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

FINERACT-842 Made TRY-IT swagger button work #1169

Merged
merged 1 commit into from Jul 20, 2020

Conversation

thesmallstar
Copy link
Member

This PR can only be merged after: #1163

@thesmallstar
Copy link
Member Author

After #1163 is merged we only need to add the following code to fineract-input.yaml ( can be converted from json to yaml automatically)

servers:
- url: /fineract-provider/api/v1
components:
  securitySchemes:
    basicAuth:
      type: http
      scheme: basic 

And the try it button would work, I have tested this :D

@thesmallstar thesmallstar changed the title FINERACT-842 Makes TRY-IT swagger button work FINERACT-842 Made TRY-IT swagger button work Jul 11, 2020
@vorburger
Copy link
Member

@ptuomola do you want to review and locally test this one? I'm happy to just merge it if it has your LGTM... (or even let this be one of the PRs that you can merge yourself, soon).

@vorburger
Copy link
Member

BTW I don't suppose there's an easy better way for that SecurityRequirement annotation to be specified only once (perhaps programmatically?), instead of having to be specified everywhere? It's not a huge deal, and I don't mind merging it as is, but was just thinking that people may forget to add that in new *Resource classes for new APIs in the future. Or perhaps they won't, because they'll just copy/paste anyway.. ;-)

@thesmallstar
Copy link
Member Author

With my basic knowledge on this( and a lot of research :P ) I could find the only this way to work.
I will search for a better option for this once more :D

@thesmallstar
Copy link
Member Author

https://www.loom.com/share/8eb964074579491090db5bacbe5eff99

@vorburger @ptuomola I have a small demo video on how this works :)

The only problem I see is to give a proper error msg if the user try to access the API without authorizing.

@thesmallstar
Copy link
Member Author

For the problem that if the user does not authorize, I think a better approach will be to have a the authorization parameters set by default to:
username: mifos
password: password

Since this would probably be used most (if not all) of the time for that username.
This, however, is not part of this PR and I would raise a separate PR for that purpose.

@thesmallstar thesmallstar marked this pull request as draft July 14, 2020 01:29
@thesmallstar
Copy link
Member Author

Just noticed, Some API routes are not covered yet, converting to draft to avoid accidental merge.

@thesmallstar
Copy link
Member Author

@vorburger there was an easier (and probably the correct way) to do this. I have updated this.
I have also added tenantid as an authorization param so that user can enter it if he wants.

By default, the user gets them set, and he can directly test the API :)

image

Ready for your review @ptuomola

Copy link
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

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

Oh! Wow. Ok, this is really cool... ;-) LGTM! I'd be (very) happy to merge this.. but haven't actually locally tested it myself - I'll let @ptuomola LGTM this.

@vorburger
Copy link
Member

@thesmallstar wait but can the end-user actually CHANGE the uid/pwd (and - tenant) in the Swagg UI? That wasn't clear from the screenshot, to me.

@thesmallstar
Copy link
Member Author

@vorburger yes, the initial values are only added for convenience since 90percent if the times we will probably need default id-pass and tenant.

- Check about ERROR codes [here](/fineract-provider/api-docs/apiLive.htm#errors)

Please refer to the [old documentation](/fineract-provider/api-docs/apiLive.htm) for any documentation queries
termsOfService: 'https://demo.fineract.dev/fineract-provider/api-docs/apiLive.htm'
Copy link
Member

Choose a reason for hiding this comment

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

@thesmallstar could you remove this termsOfService as discussed in #1163 ?

@thesmallstar
Copy link
Member Author

@ptuomola WDYT?

@ptuomola
Copy link
Contributor

@thesmallstar Looks great to me! Tried it out locally and it works fine

@thesmallstar
Copy link
Member Author

@vorburger this is ready to be merged 😄

@Sanyam96
Copy link
Member

LGTM
Awesome work @thesmallstar

@vorburger vorburger merged commit a84edce into apache:develop Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants