appsembler / figures

Reporting and data retrieval app for Open edX
MIT License
44 stars 37 forks source link

Make the integration with Open edX more seamless #40

Closed OmarIthawi closed 4 years ago

OmarIthawi commented 5 years ago

Going through the README I see several requests to edit the settings. I'd like to suggest better alternatives:

pip install figures That's great! It's much faster than git!


    "ADDL_INSTALLED_APPS": [
        "figures"
    ]

This is acceptable practice, especially if there's migrations (such as in this case).


    "FEATURES": {
        ... 
        "ENABLE_FIGURES": true,
        ...
    }

In my opinion this is redundant, since we're adding the app to ADDL_INSTALLED_APPS, there's no need to have a feature, it's not like being a feature inside the edX platform.


from figures.settings import FIGURES

Please avoid this pattern, there's about 200 dependencies/apps in the edX Platform almost none of them uses this pattern. This pattern is problematic since Figures depends on the settings and I don't see any need why settings would depend on Figures.


    if FEATURES.get('ENABLE_FIGURES'):
        from figures.settings import FIGURES

Sames goes here.

if settings.FEATURES.get('ENABLE_FIGURES'):
        urlpatterns += (
            url(r'^figures/',
                include('figures.urls', namespace='figures')),
        )

if settings.FEATURES.get('ENABLE_FIGURES'): -> 'figures' in if settings.INSTALLED_APPS'):


The way static files is integrated should be completely decoupled from the edX Platform such as:

  1. The platform should know nothing about how the static files are manages.
  2. Figures should know nothing about how the platform manages its static files.
  3. Static assets such as compiled js and css files should be committed to the git repo.
  4. Figures views should call those files, the XBlock ResourceLoader is a good example of such pattern.
OmarIthawi commented 5 years ago

cc: @johnbaldwin

johnbaldwin commented 5 years ago

@OmarIthawi

Webpack loader dependency:

Can you suggest an alternate solution that will add Figures to the LMS's WEBPACK_LOADER setting? The Figures UI is a Create React App based project. It is served with Django Webpack Loader. I'm sure there is a way to serve up compiled Figures assets w/out Webpack Loader, but I'm not yet familiar enough with how "Create React App"/Webpack is compiling the assets and setting dependencies to not need Django Webpack loader.

Celerybeat schedule:

Can you suggest an alternate solution to adding Figures to the celerybeat scheduler settings in the LMS envs? AFAIK, Celery 3.1 doesn't include dynamic schedule creation w/out adding a third party app to manage the schedule. See here: http://docs.celeryproject.org/en/3.1/userguide/periodic-tasks.html

johnbaldwin commented 5 years ago

@OmarIthawi I created a new issues to address your specific comment on ENABLE_FIGURES in the FEATURES section of the server vars.

This makes sense and I can check if Figures is in the INSTALLED_APPS to see if we're going to load Figures settings in order to add entries to WEBPACK_LOADER and CELERYBEAT_SCHEDULE

I'll update the Ginkgo and Ficus Appsembler feature branches and the README to reflect this

l1ph0x commented 5 years ago

Apologies in advance, if my comment below does not belong here, and if it is an unrelated issue. I am running a native install of open-release/ginkgo.master. I have installed Appsembler's Figures following the How To for production env found here. After successfully migrating and restarting the services figures tab does not show under user's (staff) tab. I compiled (java script) assets, too. Still no effect. I think WebPack somehow does not load it but I do not know how to debug it.

OmarIthawi commented 5 years ago

@l1ph0x there's no tab yet. Perhaps you'd like to create a new issue for it.

l1ph0x commented 5 years ago

@OmarIthawi Thanks a lot for your reply. I saw a demo on Appsembler's website. And that is where my assumption came from. So far everything works very well on my install of Appsembler's Figures. So, I had better wait for Devs to decide whether there is going a tab or not, and if yes, where it would go.

OmarIthawi commented 5 years ago

@l1ph0x Good catch! It's something that you'd have to edit in your fork. As far as I know Open edX doesn't allow extending the dropdown, except by modifying the theme.

OmarIthawi commented 4 years ago

This has been resolved. Closing.