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

perf: quince release support #47

Merged
merged 14 commits into from
Feb 9, 2024
Merged

Conversation

luisfelipec95
Copy link
Contributor

@luisfelipec95 luisfelipec95 commented Jan 25, 2024

Description

This PR adds support for the Open edx Quince release.

Testing instructions

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

Aditional information

JIRA ISSUE DS-778

@Alec4r Alec4r requested review from dcoa and MaferMazu January 25, 2024 04:36
@luisfelipec95 luisfelipec95 changed the title fix: update urls in favor of re_path for deprecation perf: quince release support Jan 25, 2024
@luisfelipec95 luisfelipec95 requested a review from a team January 26, 2024 14:48
@bra-i-am
Copy link

bra-i-am commented Jan 29, 2024

hi @luisfelipec95, I hope you're doing well

I've been testing this PR (I'm just testing the functionality and haven't seen the code yet), but something is going wrong with the execution; did you follow the EOX hooks test cases to the letter, or did something different that we should be aware of?

Some examples:

  1. This error happened to me trying to run the test case number 3

image

  1. And I'm not being able to unenroll from a course with any account trying to run the test case number 6

@luisfelipec95
Copy link
Contributor Author

hi @luisfelipec95, I hope you're doing well

I've been testing this PR (I'm just testing the functionality and haven't seen the code yet), but something is going wrong with the execution; did you follow the EOX hooks test cases to the letter, or did something different that we should be aware of?

Some examples:

1. This error happened to me trying to run the [test case number 3](https://docs.google.com/document/d/1f7LkXyxUbBcAdkDsWhtEzNfVNFFmV65enpKrEUUnirw/edit#heading=h.oas0hz2ofaux)

image

2. And I'm not being able to unenroll from a course with any account trying to run the [test case number 6](https://docs.google.com/document/d/1f7LkXyxUbBcAdkDsWhtEzNfVNFFmV65enpKrEUUnirw/edit#heading=h.vm6mtsicj7jx)

Thanks, I already made a fix

@bra-i-am
Copy link

bra-i-am commented Jan 30, 2024

Thank you so much, @luisfelipec95, now the main functionality is working as expected.

Now, reviewing the code & docs I have some comments:

  1. Despite the labeler workflow is not failing, the Label is not being applied properly; I would recommend updating the dependency to codelytv/[email protected]
  2. Remember to update the constraints.txt, e.g, now django should be something like Django<5.0 because edx-platform now uses Django==4.2.8
  3. And would be great if in the test cases file and in this PR you specify that is not possible to test the Test Certificate Created yet if is enable the learner-dashboard MFE (here is the way to disable it)

@github-actions github-actions bot added the size/m label Feb 5, 2024
@luisfelipec95
Copy link
Contributor Author

Thank you so much, @luisfelipec95, now the main functionality is working as expected.

Now, reviewing the code & docs I have some comments:

1. Despite the `labeler` workflow is not failing, the Label is not being applied properly; I would recommend updating the dependency to `codelytv/[email protected]`

2. Remember to update the [constraints.txt](https://github.com/eduNEXT/eox-hooks/blob/master/requirements/constraints.txt), e.g, now django should be something like `Django<5.0` because edx-platform now uses [Django==4.2.8](https://github.com/openedx/edx-platform/blob/9ca3c694685b59168dd9956a50b89c089f490e32/requirements/edx/base.txt#L184)

3. And would be great if in the [test cases file](https://docs.google.com/document/d/1f7LkXyxUbBcAdkDsWhtEzNfVNFFmV65enpKrEUUnirw/edit) and in this PR you specify that is not possible to test the [Test Certificate Created](https://docs.google.com/document/d/1f7LkXyxUbBcAdkDsWhtEzNfVNFFmV65enpKrEUUnirw/edit#heading=h.oas0hz2ofaux) yet if is enable the learner-dashboard MFE ([here is the way to disable it](https://github.com/overhangio/tutor-mfe?tab=readme-ov-file#disabling-individual-mfes))

Thank you very much!! I've already made the corrections.

bra-i-am
bra-i-am previously approved these changes Feb 5, 2024
Copy link

@bra-i-am bra-i-am left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

  • You update the chore requirements.
  • Updated the Github actions.
  • Update the requirements.
    • Sorry for my late review, but I think we need to modify the constraints. If, in the constraint, we have something like djangorestframework<3.13.0 or opened-events==4.1.1, we are forced to use that version instead of the version by default from quince (restframework and events version), at least in our GitHub tests. I recommend looking at those constraints and maintaining only the necessary ones, trying to avoid the super strict constraints == (unless required).
  • Update the readme and other doc.
    • I think we need to update a little bit the format of the quince code block a (add lines between the content and the code-block because that info doesn't appear correctly in the readme), if it makes sense maybe group the release with similar config, for example Palm, you can say Palm and Quince, and that's it.
  • The test cases work.

And again, sorry for not giving my feedback before.

@luisfelipec95
Copy link
Contributor Author

Thanks for this, @luisfelipec95.

* [x]  You update the chore requirements.

* [x]   Updated the Github actions.

* [ ]   Update the requirements.
  
  * Sorry for my late review, but I think we need to modify the constraints. If, in the constraint, we have something like djangorestframework<3.13.0 or opened-events==4.1.1, we are forced to use that version instead of the version by default from quince ([restframework](https://github.com/openedx/edx-platform/blob/open-release/quince.master/requirements/edx/base.txt#L382) and [events](https://github.com/openedx/edx-platform/blob/open-release/quince.master/requirements/edx/base.txt#L780) version), at least in our GitHub tests. I recommend looking at those constraints and maintaining only the necessary ones, trying to avoid the super strict constraints == (unless required).

* [ ]   Update the readme and other doc.
  
  * I think we need to update a little bit the format of the quince code block a (add lines between the content and the code-block because that info [doesn't appear correctly in the readme](https://github.com/eduNEXT/eox-hooks/blob/lfc/quince_release_support/README.rst)), if it makes sense maybe group the release with similar config, for example Palm, you can say Palm and Quince, and that's it.

* [x]   The test cases work.

And again, sorry for not giving my feedback before.

Thank you very much for the feedback

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. It looks good to me.

Copy link

@bra-i-am bra-i-am left a comment

Choose a reason for hiding this comment

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

Comments addressed & working as expected!
Thanks ✨

@luisfelipec95 luisfelipec95 merged commit 74cbf8e into master Feb 9, 2024
4 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.

4 participants