Skip to content

Adding plugin code from edx-platform - #48

Merged
jinder1s merged 28 commits into
masterfrom
msingh/extract/plugins_infra
Aug 10, 2020
Merged

Adding plugin code from edx-platform#48
jinder1s merged 28 commits into
masterfrom
msingh/extract/plugins_infra

Conversation

@jinder1s

@jinder1s jinder1s commented Aug 4, 2020

Copy link
Copy Markdown
Contributor

PR in edx-platform that tests this move: https://github.com/edx/edx-platform/pull/24678

previous location in edx-platform: https://github.com/edx/edx-platform/tree/master/openedx/core/djangoapps/plugins

This is mostly a drop in replacement, though with some link changes and I ran black on plugin directory.
Link to working sandbox: https://plugless.sandbox.edx.org/

Description:

Describe in a couple of sentence what this PR adds

JIRA:

XXX-XXXX

Dependencies:

List dependencies on other outstanding PRs, issues, etc.

Merge deadline:

List merge deadline (if any)

Installation instructions:

List any non-trivial installation
instructions.

Testing instructions:

  1. Open page A
  2. Do thing B
  3. Expect C to happen
  4. If D happened instead - check failed.

Reviewers:

  • @edx/arch-review (Required)
  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed
  • PR author is listed in AUTHORS

Post merge:

  • Create a tag
  • Check new version is pushed to PyPi after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns:

List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

@jinder1s
jinder1s marked this pull request as ready for review August 4, 2020 19:28

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @jinder1s.

  1. I didn't really look at code, because I presume you didn't touch it. Let me know if you made changes and what I should look for.
  2. I don't have any background on "Plugin Contexts" and how baked it is, and whether it should be marked Accepted in the ADR and should be moved along with everything else. :)

UPDATE: Looks like "Plugin Contexts" are marked as Adopt, so we should be all set in moving it as well. See https://github.com/edx/edx-platform/pull/24057/files#r465341119

Comment thread docs/decisions/0001-purpose-of-this-repo.rst Outdated
Comment thread docs/decisions/0001-purpose-of-this-repo.rst
Comment thread docs/decisions/0001-purpose-of-this-repo.rst
Comment thread docs/decisions/0001-purpose-of-this-repo.rst Outdated
Comment thread docs/decisions/0002-extracts-plugins-infrastructure-from-edx-platform.rst Outdated
Comment thread edx_django_utils/plugins/docs/decisions/0003-plugin-contexts.rst
Comment thread edx_django_utils/plugins/docs/decisions/0003-plugin-contexts.rst
Comment thread edx_django_utils/plugins/plugin_manager.py
Comment thread edx_django_utils/plugins/plugin_manager.py Outdated
Comment thread edx_django_utils/plugins/docs/decisions/0003-plugin-contexts.rst
@jinder1s
jinder1s force-pushed the msingh/extract/plugins_infra branch from 0f8c137 to a498191 Compare August 5, 2020 15:12

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some final details before approval:

  • See comment
  • Temporarily omit files from codecov with TODO comment pointing to ticket that will add coverage.
  • Codecov has been having issues on this repo. Is that a global issue, or does config for this repo need to be reviewed, or do we need to open a ticket with them?
  • Please add PR comment when you have successful sandbox using plugin.

Comment thread tox.ini
@jinder1s

jinder1s commented Aug 6, 2020

Copy link
Copy Markdown
Contributor Author
  • Temporarily omit files from codecov with TODO comment pointing to ticket that will add coverage.

Can you elaborate more on what you mean by "add coverage"? @robrap
Also, by omit files, you mean add plugins directory to omit in .coveragerc. Or should I comment out the patch section in codecov.yml?

@robrap

robrap commented Aug 6, 2020

Copy link
Copy Markdown
Contributor

@jinder1s: I mean adding tests.

@robrap

robrap commented Aug 7, 2020

Copy link
Copy Markdown
Contributor

You could add the pragma for no cover (search for ‘pragma’) to the if statement below to bump up coverage for the project.

38 | 4 | declared_middleware = getattr(settings, 'MIDDLEWARE', None)
39 |   | if declared_middleware is None and django.VERSION[0] < 2:
40 |   | declared_middleware = settings.MIDDLEWARE_CLASSES # Pre-Django 2 support

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once you get codecov to green and have tested in LMS.

Comment thread CHANGELOG.rst Outdated
@jinder1s
jinder1s merged commit aaac995 into master Aug 10, 2020
@jinder1s
jinder1s deleted the msingh/extract/plugins_infra branch August 10, 2020 12:16
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.

2 participants