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

DS-188-eox-theming support for Nutmeg release #40

Merged
merged 5 commits into from
Sep 20, 2022

Conversation

JuanDavidBuitrago
Copy link
Member

@JuanDavidBuitrago JuanDavidBuitrago commented Sep 17, 2022

Description

This PR adds eox-theming support for Nutmeg release

Testing instructions

  1. Use this strain.yml file to create a nutmeg project
STRAIN_NAME: nutmeg
STRAIN_TUTOR_VERSION: v14.0.5
DEV_PROJECT_NAME: nutmeg_dev
LOCAL_PROJECT_NAME: nutmeg_local
CMS_HOST: studio.nutmeg.edunext.link
LMS_HOST: lms.nutmeg.edunext.link
PLATFORM_NAME: Distro nutmeg
STRAIN_TUTOR_PLUGINS:
  - REPO: tutor-contrib-edunext-distro
    VERSION: v3.0.0-rc1
    EGG: tutor-contrib-edunext-distro==v3.0.0-rc1
    PACKAGE_NAME: distro
    PROTOCOL: ssh
STRAIN_EXTRA_COMMANDS:
  - APP: 'tutor'
    COMMAND: 'distro enable-themes'
STRAIN_VOLUME_OVERRIDES:
  edxapp:
    - DESTINATION: ../../src/edxapp/edx-platform
      LOCATION: /openedx/edx-platform
    - DESTINATION: ../../src/edxapp/eox-tenant
      LOCATION: /openedx/extra_deps/eox-tenant
    - DESTINATION: ../../src/edxapp/eox-theming
      LOCATION: /openedx/extra_deps/eox-theming
STRAIN_EXPORT_VOLUMES:
  - CONTAINER: lms
    LOCATION: /openedx/edx-platform
    DESTINATION: src/edxapp/edx-platform
  - CONTAINER: lms
    LOCATION: /openedx/extra_deps/eox-tenant
    DESTINATION: src/edxapp/eox-tenant
  - CONTAINER: lms
    LOCATION: /openedx/extra_deps/eox-theming
    DESTINATION: src/edxapp/eox-theming
DISTRO_EOX_THEMING_DPKG:
  index: git
  name: eox-theming
  private: false
  repo: eox-theming
  domain: github.com
  path: eduNEXT
  protocol: https
  variables:
    development: {}
    production: {}
  version: JDB/nutmeg_release_support
  1. Run to create project
stack strain dev configure -k export_vols -k override_vols
stack strain dev init
stack strain dev configure -s export_vols -s override_vols
stack strain dev start
  1. Use ednx-test-themes repo to test eox-theming
cd src/edxapp/edx-platform/themes
git clone git@github.com:eduNEXT/ednx-test-themes.git
  1. In the file env/apps/openedx/settings/lms/development.py you must configure the appropriate path
COMPREHENSIVE_THEME_DIRS.extend(["/openedx/edx-platform/themes/ednx-test-themes/edx-platform"])
  1. To test each of the cases, the settings shown must be attached to the settings of the microsite, if you don't have a microsite create in lms.nutmeg.edunext.link:8000/admin/eox_tenant/microsite/ and adds in subdomain key lms.nutmeg.edunext.link
{
    "EDNX_USE_SIGNAL": true,
    "PLATFORM_NAME": "TEST-THEMING",
    "SITE_NAME": "lms.nutmeg.edunext.link",
    "THEME_OPTIONS": {
        "scripts": {
            "/login": [
                {
                    "content": "alert('This is a test for the inline script');",
                    "type": "inline"
                }
            ]
        },
        "theme": {
            "grandparent": "test-3",
            "name": "test-1",
            "parent": "test-2"
        }
    },
    "course_org_filter": [
        "edX"
    ]
}
  1. Go to browser lms.nutmeg.edunext.link:8000 and check the theme, something like https://github.com/eduNEXT/ednx-test-themes/blob/master/captures/1-not-auth.png
  2. Go to /login and check extra-scripts, something like https://github.com/eduNEXT/ednx-test-themes/blob/master/captures/extra-scripts.png
  3. Change microsite settings, theme color have to be diferent. Test steps 6. and 7. again. You find something like https://github.com/eduNEXT/ednx-test-themes/blob/master/captures/2-not-auth.png
        "theme": {
            "grandparent": "test-1",
            "name": "test-3",
            "parent": "test-2"
        }
  1. Go to lms.nutmeg.edunext.link:8000/account/settings and click on Reset Your Password, the file generated inside the lms container, in the path /tmp/openedx/emails/
tutor dev dc exec lms bash
cd ../../tmp/openedx/emails/

Open the file and ckeck something like https://github.com/eduNEXT/ednx-test-themes/blob/master/captures/1-email.png

Checklist for Merge

  • Tested in a remote environment
  • Updated documentation
  • Rebased master/main
  • Squashed commits

@JuanDavidBuitrago JuanDavidBuitrago self-assigned this Sep 17, 2022
@JuanDavidBuitrago JuanDavidBuitrago force-pushed the JDB/nutmeg_release_support branch from 655ece3 to fd8604d Compare September 17, 2022 13:09
@JuanDavidBuitrago JuanDavidBuitrago marked this pull request as ready for review September 19, 2022 13:05
@JuanDavidBuitrago JuanDavidBuitrago force-pushed the JDB/nutmeg_release_support branch from fd8604d to a2bf7bb Compare September 19, 2022 14:02
Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

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

In this PR, you update the requirements, but they are not being used. In addition, eox-tenant is a requirement of eox-theming and is not within the requirements in base.in

We should add eox-tenant in base.in since it is a core dependency. Ref: OEP-18: Identify Usage Context, and if you add eox-tenant we need to update req again: make update
And to add that the requirements are installed in the setup, here some reference:
eduNEXT openedx plugin cookiecutter
OpenedX cookiecutter
OEP-18: Declare Direct Dependencies

Other thing we need to check is the contrains.txt; for example in this PR we have pylint==2.12.2, and in the platform is pylint==2.13.5
What the contrains file does is restrict some reqs from being updated. The approach I would recommend here is to first compare the platform constraints with the current ones, and leave only the necessary constraints. Then update and check if the reqs of our plugin are far from the platform, I would add them in constrain with the version of the platform.

@JuanDavidBuitrago
Copy link
Member Author

In this PR, you update the requirements, but they are not being used. In addition, eox-tenant is a requirement of eox-theming and is not within the requirements in base.in

We should add eox-tenant in base.in since it is a core dependency. Ref: OEP-18: Identify Usage Context, and if you add eox-tenant we need to update req again: make update And to add that the requirements are installed in the setup, here some reference: eduNEXT openedx plugin cookiecutter OpenedX cookiecutter OEP-18: Declare Direct Dependencies

Other thing we need to check is the contrains.txt; for example in this PR we have pylint==2.12.2, and in the platform is pylint==2.13.5 What the contrains file does is restrict some reqs from being updated. The approach I would recommend here is to first compare the platform constraints with the current ones, and leave only the necessary constraints. Then update and check if the reqs of our plugin are far from the platform, I would add them in constrain with the version of the platform.

@MaferMazu Thanks for your review, please could you check the new changes?

@MaferMazu
Copy link
Contributor

  • ✅ The requirements are now being used, and they look good, although I think you missed coverage.
  • ✅ Regarding the backends they are perfect.
  • ✅ Regarding the tests that you added in the PR, everything worked perfectly for me
  • Maybe it would be good to add that microsite configuration you proposed in the test-case doc and also add the other variables that need to be added in the microsite to make it work
  • Maybe it would be good to drop the circleci tests, and add tox that tests are done with python 3.10 (just like in the github workflow so that it goes with the support windows)
  • And maybe add a note that Koa was not tested, but this is optional, could be discussed later

@JuanDavidBuitrago JuanDavidBuitrago force-pushed the JDB/nutmeg_release_support branch from 2a24811 to 4d41f34 Compare September 20, 2022 03:17
@JuanDavidBuitrago JuanDavidBuitrago force-pushed the JDB/nutmeg_release_support branch from 4d41f34 to 3c2d54f Compare September 20, 2022 03:34
@JuanDavidBuitrago
Copy link
Member Author

  • white_check_mark The requirements are now being used, and they look good, although I think you missed coverage.

    • white_check_mark Regarding the backends they are perfect.

    • white_check_mark Regarding the tests that you added in the PR, everything worked perfectly for me

    • Maybe it would be good to add that microsite configuration you proposed in the test-case doc and also add the other variables that need to be added in the microsite to make it work

    • Maybe it would be good to drop the circleci tests, and add tox that tests are done with python 3.10 (just like in the github workflow so that it goes with the support windows)

    • And maybe add a note that Koa was not tested, but this is optional, could be discussed later

@MaferMazu, I made some changes with the last comment.

@JuanDavidBuitrago JuanDavidBuitrago force-pushed the JDB/nutmeg_release_support branch 3 times, most recently from afea644 to abc4251 Compare September 20, 2022 13:03
@JuanDavidBuitrago JuanDavidBuitrago force-pushed the JDB/nutmeg_release_support branch from abc4251 to f2b3e35 Compare September 20, 2022 13:10
@JuanDavidBuitrago JuanDavidBuitrago merged commit 5313f69 into master Sep 20, 2022
JuanDavidBuitrago added a commit that referenced this pull request Sep 20, 2022
* perf: add compatibility with openedx nutmeg release

* ci(circleci): remove ci and update github actions

* build: update requirements and tox

* docs: add nutmeg info to README file

* build: update requirements
JuanDavidBuitrago added a commit that referenced this pull request Sep 20, 2022
BREAKING CHANGE: remove support version v3.1.0

* perf: add compatibility with openedx nutmeg release

* ci(circleci): remove ci and update github actions

* build: update requirements and tox

* docs: add nutmeg info to README file

* build: update requirements
@JuanDavidBuitrago JuanDavidBuitrago deleted the JDB/nutmeg_release_support branch October 10, 2022 20:45
@bra-i-am bra-i-am restored the JDB/nutmeg_release_support branch November 8, 2023 20:17
@bra-i-am bra-i-am deleted the JDB/nutmeg_release_support branch November 8, 2023 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants