-
Notifications
You must be signed in to change notification settings - Fork 258
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
Paul/paybis buy tracking #5270
Paul/paybis buy tracking #5270
Conversation
@@ -58,6 +58,7 @@ | |||
<data android:scheme="https" /> | |||
<data android:host="deep.edge.app" /> | |||
<data android:host="dl.edge.app" /> | |||
<data android:host="dp.edge.app" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been thinking about this a bunch, and I've realized return.edge.app
would be a much better domain name.
Basically, return.edge.app
would have your super-simple landing page with the "Return to Edge" button, regardless of what the URL path is. That way, if we have other services that need to return to Edge, those can use the same domain without having to hard-code any special logic into the referral server. So whether it's staking, wallet connect, or whatever other scenario comes up, we just use https://return.edge.app/stakingComplete/...
, and we get the "Return to Edge" landing page out of the box.
The "Return to Edge" button would have the same URL as the page itself, but with url.replace('return.edge.app, 'deep.edge.app)'
. That way tapping the button takes us where we need to go. If Edge isn't installed for some reason, the user will see the original "deep.edge.app" page, which has instructions for how to download the app.
For the purposes of this PR, just search & replace "dp.edge.app" with "return.edge.app", and everything else stays the same. It's a pretty small change. For the referral server, it's the same search & replace, plus changing the page-switch logic to trigger off the domain name instead of the path.
2eb1585
to
047262a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two teeny cleanups, but overall this is quite nice.
When you rebase this, be sure to rename "dp.edge.app" to "return.edge.app" in the commit messages.
providerId, | ||
deeplinkHandler: async link => { | ||
const { query, uri } = link | ||
console.log('Paybis WebView launch buy success: ' + uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover logging?
} else if (transactionStatus === 'failure') { | ||
await showUi.showToast(lstrings.fiat_plugin_buy_failed_try_again, NOT_SUCCESS_TOAST_HIDE_MS) | ||
} else { | ||
await showUi.showError(new Error('Paybis: Invalid transactionStatus. This should never happen')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe do it this way, so it's more debuggable: showUi.showError(new Error('Paybis: Invalid transactionStatus "${transactionStatus}".'))
/rebase |
047262a
to
ec86c97
Compare
`invoice` is not available at the point of a buy order so we can't use it as the orderId.
ec86c97
to
af0ef8b
Compare
af0ef8b
to
e489742
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have: