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

Make Stripe OAuth functional in event wizard #1259

Merged
merged 1 commit into from Jun 13, 2018

Conversation

3 participants
@pradeepgangwar
Copy link
Member

commented Jun 8, 2018

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Short description of what this resolves:

Currently in event creation wizard organizer do not have support to add their stripe account to enable payments. This PR makes connect to stripe button functional.

Changes proposed in this pull request:

  • Adds a new library torii which makes OAuth implementation simpler.
  • Defines new functions and methods to make stripe button functional.

Fixes #1180

@pradeepgangwar

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2018

Anybody reviewing current implementation please take care of following points. Features and modifications left:

  • Disconnect button does not work. A function is to be implemented.
  • Code has been not checked for corner cases. I will work on this soon.

@pradeepgangwar pradeepgangwar force-pushed the pradeepgangwar:stripe-final branch from 9fc222a to 1e5ce42 Jun 8, 2018

@fossasia fossasia deleted a comment from codacy-bot Jun 8, 2018

@open-event-bot

This comment has been minimized.

Copy link

commented Jun 8, 2018

Hi @pradeepgangwar!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@open-event-bot

This comment has been minimized.

Copy link

commented Jun 8, 2018

Hi @pradeepgangwar!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@@ -100,6 +100,7 @@
"qunit-dom": "^0.6.2",
"sanitize-html": "^1.14.1",
"semantic-ui-ember": "^2.1.0",
"torii": "^0.10.1",

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 9, 2018

Member

We really don't need torii for this. We only have to handle the redirect URI. That's it. Which can be done just fine on Ember. Please remove torii dependency

On second thought, we can use torii for our social logins when we implement them. Ignore my above message 😄

This comment has been minimized.

Copy link
@pradeepgangwar

pradeepgangwar Jun 9, 2018

Author Member

Okay 😅 . Only some minor things are left. I will finish this quickly so that this PR can be reviewed as a whole. Thanks.

@pradeepgangwar pradeepgangwar force-pushed the pradeepgangwar:stripe-final branch from 1e5ce42 to b04c3fb Jun 9, 2018

@fossasia fossasia deleted a comment from codacy-bot Jun 9, 2018

@pradeepgangwar pradeepgangwar changed the title [WIP] Make Stripe OAuth functional in event wizard Make Stripe OAuth functional in event wizard Jun 9, 2018

@pradeepgangwar

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2018

@niranjan94 @CosmicCoder96 @srv-twry @ritikamotwani @mayank8318 All the functionality related to stripe OAuth button seem to be working. Please review. 😄

@pradeepgangwar pradeepgangwar force-pushed the pradeepgangwar:stripe-final branch from b04c3fb to cb7eb05 Jun 9, 2018

@open-event-bot

This comment has been minimized.

Copy link

commented Jun 9, 2018

Hi @pradeepgangwar!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

torii: {
providers: {
'stripe-connect': {
clientId : process.env.STRIPE_CLIENT_ID,

This comment has been minimized.

Copy link
@srv-twry

srv-twry Jun 9, 2018

Member

This client_id is supposed to be fetched from the settings endpoint :)

This comment has been minimized.

Copy link
@pradeepgangwar

pradeepgangwar Jun 9, 2018

Author Member

@srv-twry I tried that but could not get idea about how to do it, since this file resides outside app and stripe picks its settings from this file. @niranjan94 Please help me if there is a way to do this.

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 11, 2018

Member

@pradeepgangwar you will need to extend the stripe service and customize it to make it work as per our needs.

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 11, 2018

Member

Extend the stripe-connect provider. Define the clientId property as an alias for the settings client ID property

@open-event-bot

This comment has been minimized.

Copy link

commented Jun 9, 2018

Hi @pradeepgangwar!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@pradeepgangwar pradeepgangwar force-pushed the pradeepgangwar:stripe-final branch from cb7eb05 to cac41cb Jun 9, 2018

@fossasia fossasia deleted a comment from codacy-bot Jun 9, 2018

@open-event-bot

This comment has been minimized.

Copy link

commented Jun 9, 2018

Hi @pradeepgangwar!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@fossasia fossasia deleted a comment from codacy-bot Jun 12, 2018

@pradeepgangwar pradeepgangwar force-pushed the pradeepgangwar:stripe-final branch from aa16c62 to 777a740 Jun 12, 2018


responseParams: ['code', 'state'],

clientId: configurable('clientId', async function() {

This comment has been minimized.

Copy link
@pradeepgangwar

pradeepgangwar Jun 12, 2018

Author Member

@niranjan94 I tried writing my custom provider. Everything seems to work fine. I am able to load clientId from settings here. But when I see the clientId in url (that gets formed), it is object [Object] . I printed various outputs of clientId just before return statement and it was proper string with correct clientId. But when url gets build I get it as object [Object]. I tried it for several hours with different options but none worked. I think I am missing something here. Please help.

@open-event-bot

This comment has been minimized.

Copy link

commented Jun 12, 2018

Hi @pradeepgangwar!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

import { configurable } from 'torii/configuration';
import { inject as service } from '@ember/service';

export default Oauth2.extend({

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 13, 2018

Member

Extend the stripe provider provided by torii. Just extend. Don't copy paste what is there. That will lead to code duplication. We only want to change one property there.


responseParams: ['code', 'state'],

clientId: configurable('clientId', async function() {

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 13, 2018

Member

Just make this

settings: inject(),

clientId: alias('settings.stripeClientId')

(Make sure to import alias)

@pradeepgangwar pradeepgangwar force-pushed the pradeepgangwar:stripe-final branch from 777a740 to cddc9ba Jun 13, 2018

@pradeepgangwar

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2018

@niranjan94 that worked 😄 🎆 . Thanks for the help!.
Please review.

import { inject } from '@ember/service';
import { configurable } from 'torii/configuration';

function currentUrl() {

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 13, 2018

Member

Why do you need this ?

This comment has been minimized.

Copy link
@pradeepgangwar

pradeepgangwar Jun 13, 2018

Author Member

If i don't include this. It sends redirect to /create/torii/redirect.html and gives error that it cannot parse it. Since on stripe dashboard I have given it as localhost:4200/torii/redirect.html . So either I change it on my stripe dashboard or change it here (I did this to omit the /create part from url)? Please suggest where to change.

This comment has been minimized.

Copy link
@pradeepgangwar

pradeepgangwar Jun 13, 2018

Author Member

@niranjan94 I did some more research on this and here is what is found:

  1. From docs of torii they said that

the redirect URL you register with the OAuth provider(s) that you use should be: your app base URL/torii/redirect.html

Other redirect URIs were deprecated by torii and have potential vulnerability as given here.

  1. Also if I use redirect uri as http://localhost:4200/create/torii/redirect.html it shows this error message
    screenshot from 2018-06-13 15-08-54

Please suggest me if any changes are required.

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 13, 2018

Member

That's fine. What i mean is, why do you need to override it here when it is already done at https://github.com/Vestorly/torii/blob/master/addon/providers/oauth2-code.js#L107. You don't even have to define redirectUri here. It is already part of the super class

This comment has been minimized.

Copy link
@pradeepgangwar

pradeepgangwar Jun 13, 2018

Author Member

@niranjan94 The definition given there also takes into account windows.location.pathname which will eventually include create and events/<event_id>/edit/basic-details (editing created event) before torii/redirect.html . So I thought overriding existing function so that redirect uri is always http://localhost:4200/torii/redirect.html (removed windows.location.pathname from function).
Please let me know it this can be done in any other way. It is possible I am understanding things in wrong way.

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 13, 2018

Member

Okay. Couldn't you use

return `${window.location.origin}/torii/redirect.html` 
@codecov

This comment has been minimized.

Copy link

commented Jun 13, 2018

Codecov Report

Merging #1259 into development will decrease coverage by 0.25%.
The diff coverage is 3.57%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development   #1259      +/-   ##
==============================================
- Coverage        29.16%   28.9%   -0.26%     
==============================================
  Files              318     319       +1     
  Lines             2520    2546      +26     
==============================================
+ Hits               735     736       +1     
- Misses            1785    1810      +25
Impacted Files Coverage Δ
app/routes/events/view.js 0% <ø> (ø) ⬆️
app/routes/create.js 100% <ø> (ø) ⬆️
app/models/event.js 87.5% <ø> (ø) ⬆️
app/controllers/create.js 0% <0%> (ø) ⬆️
app/torii-providers/stripe.js 0% <0%> (ø)
app/controllers/events/view/edit/basic-details.js 0% <0%> (ø) ⬆️
app/components/forms/wizard/basic-details-step.js 19.44% <14.28%> (-0.56%) ⬇️

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 6af489c...3dce4c6. Read the comment docs.

import { inject } from '@ember/service';
import { configurable } from 'torii/configuration';

function currentUrl() {

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 13, 2018

Member

That's fine. What i mean is, why do you need to override it here when it is already done at https://github.com/Vestorly/torii/blob/master/addon/providers/oauth2-code.js#L107. You don't even have to define redirectUri here. It is already part of the super class

@pradeepgangwar pradeepgangwar force-pushed the pradeepgangwar:stripe-final branch from cddc9ba to e019af8 Jun 13, 2018

@pradeepgangwar pradeepgangwar force-pushed the pradeepgangwar:stripe-final branch from e019af8 to 5ca45de Jun 13, 2018

@pradeepgangwar

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2018

@niranjan94 did the changes.
Please review.

@pradeepgangwar pradeepgangwar force-pushed the pradeepgangwar:stripe-final branch from 5ca45de to 3dce4c6 Jun 13, 2018

@open-event-bot open-event-bot bot removed the needs-review label Jun 13, 2018

@niranjan94

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

Good work!

@niranjan94 niranjan94 merged commit d7822ae into fossasia:development Jun 13, 2018

3 of 5 checks passed

codecov/patch 3.57% of diff hit (target 29.16%)
Details
codecov/project 28.9% (-0.26%) compared to 6af489c
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.