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

#496 One time links should be generated by our url shortener #615

Merged

Conversation

StanislavShevchenko
Copy link
Contributor

@StanislavShevchenko StanislavShevchenko commented Sep 18, 2019

#496

One time links should be generated by our url shortener

We’ve changed forming of links and add conditions for prod and qa/dev. We’ve added two new parameters
REACT_APP_RECEIVE_URL=
REACT_APP_SEND_URL=

For prod they should be
get.gdlr.info/
give.gdlr.info/

There is condition, if it’s Prod, then we add code after slash ( have to add other parameters as well), if dev/qa we add GET parameters.

Payment links params should stay the same on production the only the difference is the format of the link, that the code is in url other params should be the same.

For payment we can simply use dapp.gooddollar.org/?paymentCode=
for receive it should go to the .../Send?code=

And removed double encodeURI

Copy link
Contributor

@sirpy sirpy left a comment

Choose a reason for hiding this comment

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

@StanislavShevchenko re-read the issue
and address all problems raised there

@sirpy sirpy added the QA ready label Sep 19, 2019
@sirpy sirpy self-requested a review September 22, 2019 07:47
Copy link
Contributor

@sirpy sirpy left a comment

Choose a reason for hiding this comment

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

@Nordwhale @StanislavShevchenko
do we still need to make the link with the whole path? ie /Applicaiton/Dashboard/Receive?code=
or is it enough to use /?paymentCode= and /?code= ?

@StanislavShevchenko
Copy link
Contributor Author

@Nordwhale @StanislavShevchenko
do we still need to make the link with the whole path? ie /Applicaiton/Dashboard/Receive?code=
or is it enough to use /?paymentCode= and /?code= ?

Yes, now it’s enough to have a link
/? paymentCode = and /? code =?
This can be specified in environment variables:
REACT_APP_RECEIVE_URL=
REACT_APP_SEND_URL=

@sirpy sirpy reopened this Sep 23, 2019
@sirpy sirpy self-requested a review September 23, 2019 11:41
Copy link
Contributor

@sirpy sirpy left a comment

Choose a reason for hiding this comment

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

  1. why was this closed?
  2. please add unit tests that verify link is created correctly

@StanislavShevchenko
Copy link
Contributor Author

StanislavShevchenko commented Sep 23, 2019

  1. why was this closed?

Sorry, maybe clicked

@StanislavShevchenko
Copy link
Contributor Author

  1. why was this closed?
  2. please add unit tests that verify link is created correctly

done

@@ -89,9 +89,9 @@ describe('generateShareLink', () => {
const params = { key: 'value with spaces' }

// When
const link = generateShareLink(action, params)
const link = encodeURI(generateShareLink(action, params))
Copy link
Contributor

Choose a reason for hiding this comment

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

The link that we generate should be already encoded.
This suggest there are probably issues at the code where we use links that are not encoded.
I suggest we change the generateShareLink to encode the uri and remove encodeuri from other places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The generateShareObject method does encodeURI that breaks the reference in Share Object . But most likely it is necessary to remove reuse in the generateShareObject method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const Config = {
env: process.env.REACT_APP_ENV || 'development',
version: process.env.VERSION || 'v0',
logLevel: process.env.REACT_APP_LOG_LEVEL || 'debug',
serverUrl: process.env.REACT_APP_SERVER_URL || 'http://localhost:3003',
gunPublicUrl: process.env.REACT_APP_GUN_PUBLIC_URL || 'http://localhost:3003/gun',
web3SiteUrl: process.env.REACT_APP_WEB3_SITE_URL || 'https://w3.gooddollar.org',
publicUrl: process.env.REACT_APP_PUBLIC_URL || (window && window.location && window.location.origin),
publicUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is defined above in this file

src/config/config.js Outdated Show resolved Hide resolved
1 EncodeURI procedure transfer
2 Change default value
…me-links-should-be-generated-by-our-url-shortener
…nerated-by-our-url-shortener' into #496-One-time-links-should-be-generated-by-our-url-shortener
src/config/config.js Outdated Show resolved Hide resolved
src/config/config.js Outdated Show resolved Hide resolved
@sirpy sirpy merged commit 3f2e517 into master Oct 2, 2019
@sirpy sirpy deleted the #496-One-time-links-should-be-generated-by-our-url-shortener branch December 24, 2019 07:36
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.

None yet

4 participants