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

feat(DS-708): add compatibility with Open edX Palm release #100

Merged
merged 23 commits into from
Jan 2, 2024

Conversation

luisfelipec95
Copy link
Contributor

Description

This PR adds support for the Open edx Palm release. Adds issue templates.

Testing instructions

  1. Create an environment with Palm release, you can use Tutor or TVM
  2. Install the Django plugin eox-tagging, you can use steps in Tutor documentation.
  3. Run test cases

@MaferMazu
Copy link
Contributor

MaferMazu commented Dec 26, 2023

To solve the unit tests, we fix four things:

  • We tag pylint to the same version of the edx-platform to avoid errors with directories.
  • We tag newrelic with the edx-platform version.
  • We use assert_called_once_with.
  • We use filterset_class because filter_class was deleted in version 22.1 of django-filters.

References:
Newrelic in edx-platform: https://github.com/openedx/edx-platform/blob/open-release/palm.master/requirements/edx/testing.txt#L938
Pylint in edx-platform: https://github.com/openedx/edx-platform/blob/open-release/palm.master/requirements/edx/testing.txt#L1122
Assert called once function: https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_called_once_with.
Django-filter changelog: https://github.com/carltongibson/django-filter/blob/main/CHANGES.rst#version-221-2022-6-17

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.

Thanks for this, @luisfelipec95. I left you some comments to improve, but this looks good so far.

eox_tagging/settings/common.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@MaferMazu
Copy link
Contributor

I updated the readme only because django-filters renamed their functions and could break the functionality in previous versions.

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.

Thanks @luisfelipec95.

This PR looks good.

The test cases are working.
The tests are passing.
The requirements are updated.
And it works.

@MaferMazu MaferMazu merged commit 9540f2a into master Jan 2, 2024
3 checks passed
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