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.

Sign up for GitHub

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: Browser back button error in Payment page #3

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Fix: Browser back button error in Payment page #3

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f389d62
chore: updated code to handle browser back button click
zubair-ce07 Apr 5, 2024
b58e169
Merge branch 'master' into zubair-back-button-fix
zubair-ce07 Apr 5, 2024
2cc54bc
chore: updated code to handle browser back button click
zubair-ce07 Apr 8, 2024
2ef9af9
Merge branch 'zubair-back-button-fix' of github.com:openedx/frontend-…
zubair-ce07 Apr 8, 2024
6a34d3b
fix: removed extra code
zubair-ce07 Apr 8, 2024
91c877e
fix: updated code
zubair-ce07 Apr 8, 2024
d5a059c
fix: updated code to fix failing test
zubair-ce07 Apr 8, 2024
68b7c59
fix: incorporated feedback comments and update the code
zubair-ce07 Apr 16, 2024
d6e0b78
fix: removed unused code
zubair-ce07 Apr 16, 2024
7b6c18a
fix: disables eslint issues
zubair-ce07 Apr 16, 2024
e7517bd
Merge branch 'master' into zubair-back-button-fix
zubair-ce07 Apr 16, 2024
3c796a4
chore: added test case to check if skus are stored in localstorage
zubair-ce07 Apr 17, 2024
2a2e790
refactor: updated variable name
zubair-ce07 Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/payment/PaymentPage.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
import { connect } from 'react-redux';
import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/i18n';
import { AppContext } from '@edx/frontend-platform/react';
import { getConfig } from '@edx/frontend-platform';
import { sendPageEvent } from '@edx/frontend-platform/analytics';

import messages from './PaymentPage.messages';
Expand Down Expand Up @@ -42,8 +43,17 @@ class PaymentPage extends React.Component {
}

componentDidMount() {
sendPageEvent();
this.props.fetchBasket();
const sku = localStorage.getItem('sku');

// Check if SKU is not null
if (sku !== null) {
const paymentPage = `${getConfig().ECOMMERCE_BASE_URL}/basket/add/?sku=${sku}`;
window.location.href = paymentPage;
localStorage.removeItem('sku');
} else {
this.props.fetchBasket();
sendPageEvent();
}
Copy link
Member

Choose a reason for hiding this comment

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

Have a couple of questions:

  1. Can you make this logic specific to PayPal? We don't want the window location to be set to another URL for Stripe purchases.
  2. Have you tested that regular credit card (Stripe) purchases work?
  3. Have you tested what happens on re-load of the page?

Also, continuing on the other comment I added, you could dispatch an action from this component to set localStorage

Copy link
Author

Choose a reason for hiding this comment

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

Regarding point 1 I'll update the code so it is specific to PayPal only.
Point 2. Yes I've tested regular credit card purchases
Point 3. On page reload a new basket is created that contains courses/program user was about to purchase. Previously new empty basket was created.

}

renderContent() {
Expand Down
6 changes: 5 additions & 1 deletion src/payment/data/reducers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ const basket = (state = basketInitialState, action = null) => {
loaded: true,
};

case BASKET_DATA_RECEIVED: return { ...state, ...action.payload };
case BASKET_DATA_RECEIVED:
if (action.payload.products && action.payload.products.length > 0) {
localStorage.setItem('sku', action.payload.products[0].sku);
Copy link
Member

Choose a reason for hiding this comment

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

Noted you have new code to handle baskets with more than 1 product.

Apart from that, I don't think it's recommended to directly update localStorage within reducers, as they should be pure functions, without any side effects.
You can update it instead within the PaymentPage component or in actions.

Copy link
Author

Choose a reason for hiding this comment

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

Alright 👍 I'll update the code accordingly. Thanks

}
return { ...state, ...action.payload };

case BASKET_PROCESSING: return {
...state,
Expand Down
Loading