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

Paul/paybis buy tracking #5270

Merged
merged 4 commits into from
Sep 30, 2024
Merged

Paul/paybis buy tracking #5270

merged 4 commits into from
Sep 30, 2024

Conversation

paullinator
Copy link
Member

@paullinator paullinator commented Sep 21, 2024

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Requirements

If you have made any visual changes to the GUI. Make sure you have:

  • Tested on iOS device
  • Tested on Android device
  • Tested on small-screen device (iPod Touch)
  • Tested on large-screen device (tablet)

@@ -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" />
Copy link
Contributor

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.

@paullinator paullinator force-pushed the paul/paybisBuyTracking branch 2 times, most recently from 2eb1585 to 047262a Compare September 23, 2024 23:58
Copy link
Contributor

@swansontec swansontec left a 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)
Copy link
Contributor

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'))
Copy link
Contributor

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}".'))

@paullinator
Copy link
Member Author

/rebase

`invoice` is not available at the point of a buy order so we can't use it as the orderId.
@paullinator paullinator merged commit dfc7026 into develop Sep 30, 2024
2 checks passed
@paullinator paullinator deleted the paul/paybisBuyTracking branch September 30, 2024 04:57
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.

2 participants