Skip to content

Final preperations to de-fork our edx-organizations - #1213

Merged
shadinaif merged 5 commits into
mainfrom
shadinaif/remove_edx_uuid
Sep 8, 2022
Merged

Final preperations to de-fork our edx-organizations#1213
shadinaif merged 5 commits into
mainfrom
shadinaif/remove_edx_uuid

Conversation

@shadinaif

@shadinaif shadinaif commented Aug 10, 2022

Copy link
Copy Markdown

Change description

This is considered part of refactoring edx-organizations into the new tahoe-sites package

  • Remove edx_uuid from the code
  • Remove direct use of organizations auth backends
  • Fix fetching site by organization
  • Remove TAHOE_ENABLE_MULTI_ORGS_PER_SITE feature flag

Type of change

  • Bug fix (fixes an issue)
  • New feature (adds functionality)

Related issues

Related to: https://appsembler.atlassian.net/browse/RED-3274

Checklists

Development

  • Lint rules pass locally
  • Application changes have been tested thoroughly
  • Automated tests covering modified code pass

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached as necessary
  • "Ready for review" label attached and reviewers assigned
  • Changes have been reviewed by at least one other contributor
  • Pull request linked to task tracker where applicable

@github-actions

This comment has been minimized.

@shadinaif shadinaif changed the title Remove edx_uuid from the code Remove edx_uuid and organizations auth backends Aug 11, 2022
@shadinaif
shadinaif force-pushed the shadinaif/remove_edx_uuid branch from 4780712 to 811a31e Compare August 11, 2022 08:12
@github-actions

This comment has been minimized.

@coveralls

coveralls commented Aug 11, 2022

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 3008827825

  • 20 of 22 (90.91%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 49.015%

Changes Missing Coverage Covered Lines Changed/Added Lines %
openedx/core/djangoapps/appsembler/sites/tests/test_site_config_client.py 6 7 85.71%
openedx/core/djangoapps/appsembler/sites/utils.py 3 4 75.0%
Totals Coverage Status
Change from base Build 3008321883: -0.02%
Covered Lines: 110874
Relevant Lines: 226205

💛 - Coveralls

@shadinaif
shadinaif marked this pull request as ready for review August 11, 2022 08:43
@shadinaif
shadinaif marked this pull request as draft August 11, 2022 08:44

@OmarIthawi OmarIthawi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks! Amazing!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great for this pull request.

A note for the future: If we end up needing this a lot, I think it makes sense to add another API:

get_organization_for_course(course_id)

We can replace get_site_for_course into get_site_by_organization(get_organization_for_course(course_id))

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Comment thread requirements/edx/appsembler.txt Outdated
@shadinaif
shadinaif force-pushed the shadinaif/remove_edx_uuid branch 2 times, most recently from f04f74e to b3b50f7 Compare August 11, 2022 12:35
@github-actions

This comment has been minimized.

@shadinaif
shadinaif force-pushed the shadinaif/remove_edx_uuid branch from b3b50f7 to 15e61b7 Compare August 11, 2022 12:48
@github-actions

This comment has been minimized.

@shadinaif
shadinaif force-pushed the shadinaif/remove_edx_uuid branch from 15e61b7 to 07c0b37 Compare August 12, 2022 05:22
@github-actions

This comment has been minimized.

@shadinaif
shadinaif marked this pull request as ready for review August 12, 2022 06:02
@shadinaif
shadinaif requested a review from OmarIthawi August 12, 2022 06:02
@shadinaif shadinaif changed the title Remove edx_uuid and organizations auth backends Final preperations to de-fork our edx-organizations Aug 12, 2022
@shadinaif
shadinaif force-pushed the shadinaif/remove_edx_uuid branch from 07c0b37 to 13cee8b Compare August 12, 2022 06:48
@github-actions

This comment has been minimized.

@OmarIthawi OmarIthawi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks @shadinaif!

I have many requested changes. The PR overall looks good, but we must do the following:

Requested changes:

  • revert changes on legacy_amc_helpers.py to avoid breaking production AMC sites
  • undo add_organization changes
  • ask bryan to review the removal of TAHOE_ENABLE_MULTI_ORGS_PER_SITE

Comment thread openedx/core/djangoapps/appsembler/sites/tests/test_utils.py Outdated
Comment thread openedx/core/djangoapps/appsembler/sites/utils.py Outdated
Comment thread openedx/core/djangoapps/appsembler/sites/utils.py Outdated
Comment thread openedx/core/djangoapps/appsembler/tahoe_tiers/legacy_amc_helpers.py Outdated
Comment thread openedx/core/djangoapps/appsembler/sites/utils.py Outdated
Comment thread openedx/core/djangoapps/appsembler/sites/serializers.py Outdated
@OmarIthawi

Copy link
Copy Markdown

@shadinaif regarding TAHOE_ENABLE_MULTI_ORGS_PER_SITE I think we should keep it removed.

I've wrote a blog post which you can use for future reference:

@shadinaif
shadinaif force-pushed the shadinaif/remove_edx_uuid branch from 13cee8b to cfe3329 Compare August 25, 2022 11:51
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@shadinaif
shadinaif force-pushed the shadinaif/remove_edx_uuid branch from 7d30777 to 356309d Compare August 25, 2022 12:08
@github-actions

This comment has been minimized.

@shadinaif
shadinaif force-pushed the shadinaif/remove_edx_uuid branch from 356309d to c1f8f7b Compare August 25, 2022 13:01
@github-actions

This comment has been minimized.

@shadinaif
shadinaif force-pushed the shadinaif/remove_edx_uuid branch from c1f8f7b to 7b5a5be Compare August 31, 2022 10:47
@github-actions

This comment has been minimized.

@OmarIthawi OmarIthawi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks @shadinaif!! One more optional change.

Looks good!

Comment thread openedx/core/djangoapps/appsembler/api/sites.py Outdated
@shadinaif
shadinaif force-pushed the shadinaif/remove_edx_uuid branch from 7b5a5be to e64355c Compare September 6, 2022 08:49
@github-actions

This comment has been minimized.

@shadinaif
shadinaif force-pushed the shadinaif/remove_edx_uuid branch from e64355c to 383ac7b Compare September 7, 2022 15:26
@github-actions

github-actions Bot commented Sep 7, 2022

Copy link
Copy Markdown

Checking git merge conflicts against https://github.com/edx/edx-platform.git

Comparing with open-release/koa.master
Benchmark conflicts with main 111
Current conflicts 111
Summary Good work! No added conflicts.
Comparing with open-release/lilac.master
Benchmark conflicts with main 254
Current conflicts 254
Summary Good work! No added conflicts.
Comparing with open-release/maple.master
Benchmark conflicts with main 285
Current conflicts 285
Summary Good work! No added conflicts.
Comparing with open-release/nutmeg.master
Benchmark conflicts with main 293
Current conflicts 293
Summary Good work! No added conflicts.
Comparing with master
Benchmark conflicts with main 289
Current conflicts 289
Summary Good work! No added conflicts.

@shadinaif
shadinaif merged commit 27c12d3 into main Sep 8, 2022
@shadinaif
shadinaif deleted the shadinaif/remove_edx_uuid branch September 8, 2022 06:50
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