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

[WIP] OeX_Depr-112 Account pages -> micro-frontend #2419

Closed
wants to merge 5 commits into from

Conversation

UvgenGen
Copy link

OeX_Depr-112 Account pages -> micro-frontend
openedx/public-engineering#71

@UvgenGen UvgenGen self-assigned this Apr 28, 2022
@UvgenGen UvgenGen force-pushed the depr/account-pages branch 3 times, most recently from 79a14c2 to fa2d15e Compare April 28, 2022 15:46
Copy link

@dyudyunov dyudyunov left a comment

Choose a reason for hiding this comment

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

Few questions inline and pay attention to failed pylint pipelines, please


If duplicate is True, we expect context['duplicate_provider'] to contain
# pylint: disable=invalid-name

Choose a reason for hiding this comment

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

Why do we need this pylint comment?

# pylint: disable=invalid-name
def assert_third_party_accounts_state(self, request, duplicate=False, linked=None):
"""
Asserts the user's third party account in the expected state.

Choose a reason for hiding this comment

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

Docstring title should be separated from docstring body by the new line

Copy link
Author

Choose a reason for hiding this comment

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

updated

@@ -495,7 +495,7 @@ def test_login_user_info_cookie(self):

# Check that the URLs are absolute
for url in user_info["header_urls"].values():
assert 'http://testserver/' in url
assert 'http://' in url

Choose a reason for hiding this comment

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

why don't we use the domain anymore?

Copy link
Author

Choose a reason for hiding this comment

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

we don't use the domain because some urls in the header have been changed. (account and profile urls)

Copy link

@idegtiarov idegtiarov left a comment

Choose a reason for hiding this comment

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

Wow!! PR is huge and looks pretty ready to be open to the upstream. ))

Just add a comment that the timeline is going to be decided when PR will be landed there are two options in Olive or next after it.

@UvgenGen UvgenGen force-pushed the depr/account-pages branch 2 times, most recently from 2ae2c2e to ad1d14e Compare May 2, 2022 13:06
@UvgenGen UvgenGen force-pushed the depr/account-pages branch from ad1d14e to 94abf32 Compare May 2, 2022 13:48
@UvgenGen UvgenGen closed this May 2, 2022
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.

3 participants