Skip to content
This repository has been archived by the owner on Nov 4, 2024. It is now read-only.

FC-0001: ecommerce Basket Page -> micro-frontend #3718

Closed
wants to merge 1 commit into from
Closed

FC-0001: ecommerce Basket Page -> micro-frontend #3718

wants to merge 1 commit into from

Conversation

UvgenGen
Copy link

@UvgenGen UvgenGen commented May 12, 2022 •
edited
Loading

Description

According to this documentation i prepared first part with migration making the enable_microfrontend_for_basket_page column nullable.
This PR should be merged after that one #3829

Removing ecommerce's older implementation of the Django-server-side rendering of the Basket Page.

Replacing the Basket page with a new micro-frontend-based implementation: edx/frontend-app-payment

Updated the model and generated a migration making the column nullable (null=True) (#3829)
Updated SiteConfiguration model: removed enable_microfrontend_for_basket_page field. (always redirects to the MFE now).
Removed views and templates for Django-server-side Basket Page.
Updated tests.

Supporting information

openedx/public-engineering#68
Original Jira Issue: https://openedx.atlassian.net/browse/DEPR-42

PR to edx-platform: openedx/edx-platform#30377
PR with migration making the enable_microfrontend_for_basket_page column nullable: #3829

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels May 12, 2022
@openedx-webhooks
Copy link

openedx-webhooks commented May 12, 2022 •
edited
Loading

Thanks for the pull request, @UvgenGen! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@UvgenGen UvgenGen mentioned this pull request May 12, 2022
Open
@UvgenGen UvgenGen changed the title feat: ecommerce Basket Page -> micro-frontend FC-0001: ecommerce Basket Page -> micro-frontend May 12, 2022
@natabene
Copy link

Let's see how tests turn out.

@UvgenGen UvgenGen force-pushed the depr/basket_page branch from af8daa7 to cbf7414 Compare June 5, 2022 16:09
@bmtcril
Copy link

bmtcril commented Sep 23, 2022

Hi @UvgenGen sorry for the delay on this, I can help you get these merged. The direction looks good to me. Can you rebase this one and the edx-platform counterpart and fix up any conflicts? Once the tests are passing I can help get it over the line.

@UvgenGen
Copy link
Author

UvgenGen commented Sep 26, 2022 •
edited
Loading

Hi @bmtcril , thank you. I've rebased PR's, there were no conflicts.

bmtcril
bmtcril reviewed Sep 26, 2022
View reviewed changes
ecommerce/core/models.py
@@ -188,15 +188,9 @@ class SiteConfiguration(models.Model):
max_length=255,
blank=True
)
enable_microfrontend_for_basket_page = models.BooleanField(
Copy link

@bmtcril bmtcril Sep 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

These changes are going to require you to run makemigrations, and this specific change is destructive in a way that will make rolling back difficult in the case that something in this PR causes errors in production. Sadly removing this field will require a multi-release process as outlined here:

https://openedx.atlassian.net/wiki/spaces/AC/pages/23003228/Everything+About+Database+Migrations#EverythingAboutDatabaseMigrations-Howtodropacolumn

Copy link
Author

Choose a reason for hiding this comment

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

So for the first step, I have to create a PR with this field updated with null=True and generate the migration, right?
And after that this PR can be merged?

Copy link

Choose a reason for hiding this comment

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

We'll need that change to be in for tests to succeed. I'm waiting on some feedback to figure out who should approve you to have tests run on this repo, but you should be able to add that null=True migration in the meantime. Once tests are succeeding I think this will be good to merge, though we may want the edx-platform one to go through first.

Copy link
Author

@UvgenGen UvgenGen Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the reply, I'll wait for updates. I've created PR with making the enable_microfrontend_for_basket_page column nullable.
#3829

Copy link

Choose a reason for hiding this comment

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

Thanks, I've allowed tests to run on both PRs. This one failed, likely due to the migration issue, so is blocked on the other. I'm still working to get other folks to take a look, but once the other PR is merged this one will need another rebase to pass acceptance tests.

@UvgenGen UvgenGen mentioned this pull request Sep 27, 2022
Closed
@bmtcril bmtcril requested a review from a team September 29, 2022 20:32
@bmtcril
Copy link

bmtcril commented Sep 29, 2022

Hi @openedx/ecommerce-maintainers ! I've pinged you for review on these PRs, but can't ping you on the associated edx-platform one. I talked to Seth about them, so hopefully it's not a total surprise. I believe that we can do them in the order of:

1- edx-platform (openedx/edx-platform#30377)
2- The other ecomm one, which I don't have handy but is just the first step of the migration dance needed for this one to land
3- This one

My biggest remaining concern with these is that the Basket MFE is up to feature parity with the functionality being removed. Can anyone confirm that on your side?

@colinbrash
Copy link

Hello Brian!

@UvgenGen @bmtcril

I have a couple requests:

  1. The PR descriptions and commit messages have very little information about what changes are intended. Would you please you add more details about what the code changes actually are doing? For example, I see some discussion in a code change comment about a migration, but this is not mentioned in the Description!
  2. In addition to what the changes are, it would be helpful to understand what risks there are to merging. Again, sounds like there are risks mentioned in comments, but not described in the Description.
  3. We are in the middle of some time-sensitive, critical work and it will be difficult to review these PRs before we are finished (approx. 30 days or so). I'd like to understand the urgency and the dependencies between the PRs.
    1. Do these need to be merged by a particular date (e.g. is it an issue if we begin review in a month)? (cc @dianakhuang)
    2. Do we need to coordinate merging them on a particular schedule? Or as long as they are done in order, can they be merged on independent timelines?

Thanks!

@UvgenGen
feat: Replaced the Basket page with a new micro-frontend-based implem…
19cc911 
…entation.

- Removed legacy views and templates for basket page.
- Updated redirect to basket MFE.
@UvgenGen
Copy link
Author

@colinbrash Hi! I've updated the description and commit message.

@UvgenGen
Copy link
Author

description for the first part updated to #3829.

@feanil feanil closed this Nov 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bmtcril bmtcril bmtcril left review comments

Assignees
No one assigned
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@UvgenGen @openedx-webhooks @natabene @bmtcril @colinbrash @feanil