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

Support vanilla Hawthorn through Open edX plugins #84

Merged
merged 5 commits into from
Apr 10, 2019

Conversation

OmarIthawi
Copy link
Contributor

@OmarIthawi OmarIthawi commented Apr 3, 2019

Overview

To make the installation steps for Hawthorn+:

$ pip install figures=x.y.z
/edx/bin/edxapp-migrate-lms 🔥

Screenshot

Too Long; Don't Read

So, I made a lot of noise about it but finally got the time to actually get it done.

This pull request uses the Open edX plugins architecture that is supported since Hawthorn.

I dare to say that this also supports Ironwood, but it's early to say without really testing it.

  • This PR breaks a lot of tests, and I'm planning to fix them. Let's take on a quick first pass before I fully continue in testing and fixing tests. Now all of them are fixed.

Fixes #3

@OmarIthawi OmarIthawi requested a review from johnbaldwin April 3, 2019 21:52
@OmarIthawi
Copy link
Contributor Author

@lpm0073
Copy link

lpm0073 commented Apr 3, 2019

dude, that's fantastic news. thank you!!!! :D

@OmarIthawi
Copy link
Contributor Author

Thanks for passing by @lpm0073! I'm soo guilty of not making this pull request earlier!

@codecov-io
Copy link

codecov-io commented Apr 3, 2019

Codecov Report

Merging #84 into hawthorn-upgrade will decrease coverage by 1.11%.
The diff coverage is 58.37%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           hawthorn-upgrade     #84      +/-   ##
===================================================
- Coverage             89.22%   88.1%   -1.12%     
===================================================
  Files                    66      65       -1     
  Lines                  3600    3623      +23     
===================================================
- Hits                   3212    3192      -20     
- Misses                  388     431      +43
Impacted Files Coverage Δ
figures/apps.py 0% <0%> (ø) ⬆️
tests/views/base.py 68.96% <0%> (ø) ⬆️
figures/sites.py 61.4% <100%> (ø) ⬆️
figures/helpers.py 100% <100%> (ø) ⬆️
figures/permissions.py 77.14% <100%> (ø) ⬆️
tests/test_settings.py 100% <100%> (ø) ⬆️
figures/pagination.py 100% <100%> (ø) ⬆️
figures/pipeline/logger.py 100% <100%> (ø) ⬆️
tests/pipeline/test_course_daily_metrics.py 97.67% <100%> (ø) ⬆️
tests/test_pagination.py 100% <100%> (ø) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 173e4e1...eb1f81c. Read the comment docs.

@OmarIthawi OmarIthawi changed the base branch from develop to hawthorn-upgrade April 3, 2019 22:48
@OmarIthawi OmarIthawi mentioned this pull request Apr 3, 2019
1 task
figures/pagination.py Outdated Show resolved Hide resolved
@OmarIthawi OmarIthawi force-pushed the omar/no-fork branch 2 times, most recently from 1c1a6a4 to b41f7af Compare April 8, 2019 21:23
@OmarIthawi OmarIthawi changed the title (WIP) Support vanilla Hawthorn, while maintaining Ficus support (WIP) Support vanilla Hawthorn through Open edX plugins Apr 8, 2019
@OmarIthawi OmarIthawi changed the title (WIP) Support vanilla Hawthorn through Open edX plugins Support vanilla Hawthorn through Open edX plugins Apr 9, 2019
@OmarIthawi
Copy link
Contributor Author

@johnbaldwin I've rebased the branch over hawthorn-upgrade. I think it's ready to be merged.

One thing I've done, no Ficus support anymore here!

@OmarIthawi
Copy link
Contributor Author

@johnbaldwin any idea how to convince CodeCov that it's ok?

@johnbaldwin
Copy link
Contributor

@OmarIthawi I'll look into this today

@johnbaldwin
Copy link
Contributor

johnbaldwin commented Apr 9, 2019

@OmarIthawi It appears the issue is that code coverage dropped too low. We need test coverage for figures.apps. I need to look into how to unit test apps. I will add more test coverage for figures.sites and figures.permissions since those are pretty weak

@OmarIthawi
Copy link
Contributor Author

Thanks @johnbaldwin! I'm done here, as far as I know. Please merge if it looks good to you.

In multisite mode, figures.sites requires organizations to be linked
with sites and courses and courses to be linked with organizations. This
commit creates those associations if multisite mode enabled ane/or Appsembler's
fork of edx-organizations is used
Added assertions where user sets should have the same number of records.
This is failing because additional users are apparently being added to the test
database between the setup call and the test method
@johnbaldwin johnbaldwin merged commit 5acbf03 into hawthorn-upgrade Apr 10, 2019
@johnbaldwin johnbaldwin deleted the omar/no-fork branch April 10, 2019 15:03
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.

4 participants