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

fix socialpay deeplink #4972

Merged
merged 1 commit into from
Feb 19, 2024
Merged

fix socialpay deeplink #4972

merged 1 commit into from
Feb 19, 2024

Conversation

dulguun0225
Copy link
Collaborator

ISSUE

Context

Your context here. Additionally, any screenshots. Delete this line.

// Delete the below section once completed

PR Checklist

  • Description is clearly stated under Context section
  • Screenshots and the additional verifications are attached

Copy link

sonarcloud bot commented Feb 19, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Enhancement

PR Summary: This pull request introduces changes across several files with the primary goal of fixing the deeplink functionality for Social Pay within the plugin-payment-api package. It modifies constants to include trailing commas for better diff readability, updates the Social Pay API to change the source of the deeplink, and adjusts the client-side JavaScript to handle the deeplink navigation specifically for Social Pay. Additionally, it includes minor formatting improvements across various payment methods.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.

General suggestions:

  • Ensure that the change in the source of the deeplink for Social Pay from body.response.deeplink to body.response.desc does not inadvertently introduce issues with deeplink functionality. It's crucial that the desc field is validated to contain a proper URL format suitable for deeplink purposes.
  • Revisit the client-side navigation logic introduced for Social Pay in script.js. The current implementation might cause redundant navigation actions or unexpected behavior due to the simultaneous use of window.location.href and window.open. A more streamlined approach to handling deeplinks could enhance user experience and prevent potential navigation issues.
  • The PR description could be improved by providing more context about the changes made and their impact on the application. Including specific reasons for the changes and how they address the issue at hand can help reviewers understand the intent behind the PR better.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -203,6 +203,10 @@ async function onPaymentClick(payment, invoiceData, prefix) {

window.location.href = apiResponse.deeplink;
window.open(apiResponse.deeplink, 'blank');

if (paymentObj.kind === 'socialpay') {
window.location.href = apiResponse.deeplink;
Copy link

Choose a reason for hiding this comment

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

issue (llm): The addition of a conditional redirect specifically for socialpay after already setting window.location.href and opening a new tab with window.open could lead to unexpected behavior or user experience issues. Consider consolidating the navigation logic to ensure a consistent and predictable outcome.

@@ -116,7 +116,7 @@ export class SocialPayAPI extends BaseAPI {

const qrData = await QRCode.toDataURL(body.response.desc);

return { qrData, deeplink: body.response.deeplink };
return { qrData, deeplink: body.response.desc };
Copy link

Choose a reason for hiding this comment

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

issue (llm): Returning body.response.desc as the deeplink instead of body.response.deeplink appears to be a significant change. If this is intentional, ensure that the desc field is correctly formatted and can serve as a deeplink. This change could potentially break functionality if desc does not contain a URL or is not intended to be used as a deeplink.

@dulguun0225 dulguun0225 merged commit 2b4041b into dev Feb 19, 2024
3 of 4 checks passed
@dulguun0225 dulguun0225 deleted the update-socialpay branch February 19, 2024 05:53
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

2 participants