Initial commit of Django application boilerplate and base models #1

johnbaldwin · 2017-12-04T16:56:07Z

This PR is a starting point that covers three areas

Project scaffolding (setup/package structure/readme)
The models to which we will save learner report data for report generation and retrieving time series data
Skeleton/starting point for the front end code as a React app. We're using Django Webpack Loader as glue. See the edx-platform PR: Support Django Webpack Loader and edx-figures edx-platform#189

The plan is after we get this reviewed/comments addressed, we'll move forward and add reporting, testing, then UI

Some details:

Boilerplace app code generated with the Django startapp command
The README file that provides a start of the installation steps for review
Learner report and learner report row data models. Please read the inline
documentation in the models.py
Baseline default settings in settings.py
App config in apps.py
Baseline setup.py
Generated migrations for the existing models
.gitignore file generated by github

This commit adds the following * Boilerplace app code generated with the Django `startapp` command * The README file that provides a start of the installation steps for review * Learner report and learner report row data models. Please read the inline documentation in the models.py * Baseline default settings in settings.py * App config in apps.py * Baseline setup.py * Generated migrations for the existing models * .gitignore file generated by github

johnbaldwin · 2017-12-04T16:57:00Z

(3) add react/webpack hooks to edx-figures for Matej

OmarIthawi · 2017-12-05T17:30:12Z

@johnbaldwin how much critique are you willing (i.e. have time) to address. I mean, for sure comments like "Lets drop webpack" are not welcome, right?

grozdanowski · 2017-12-07T18:59:35Z

@bezidejni Can you also take a look at this?

johnbaldwin · 2017-12-07T22:51:40Z

@omar I'm open to "let's drop webpack" (really this is about this code uses tool X and I want it to use tool Y) under two conditions

You make a really good case why X is fundamentally the wrong tool choice to go and Y is fundamentally the right tool choice. What are the fatal flaws in X that are not in Y. Note: Y is going to have some of it's own fatal flaws, which brings us to Add installation instructions #2
If it seems that there's a good case for getting rid of X and replacing it with Y, I'll assume you know a lot about both, which means: Write a PR that replaces X functionality with Y

Oh, also #3 : How does X or Y for that matter fit with the direction which Open edX is in and how well does it play with Django?

johnbaldwin · 2017-12-07T22:53:12Z

@OmarIthawi This is partly why I want to do this package a layer at a time, so that we can have these kinds of discussions and help, as a team, to shape a robust system

OmarIthawi · 2017-12-08T04:47:17Z

.gitignore

@@ -0,0 +1,104 @@
+# Byte-compiled / optimized / DLL files


Please add .idea, makes it easier for code newbies like me who uses PyCharm (vs being super super competent and using vim 😄).

@OmarIthawi Sure, added

OmarIthawi · 2017-12-08T04:49:08Z

README.rst

+
+1. Install edx-figures. ``pip install edx-figures``
+
+2. Add the following to ``lms.env.json``::


lms.env.json is meant to be machine written. Let's say (or Ansible server-vars.yml) here to help Open edX newbies not to get lost.

@OmarIthawi I've run into a number of occasions where the official docs refer to manually updating the env/auth json files. While I agree that in a proper production deployment environment, these are automatically generated, it seems in practice this is not always the case in the community. Therefore, I'll add mention to add vars to files included with --extra-vars

@OmarIthawi

A question: is server-vars.yml a common accepted community term for files the playbook includes via --extra-vars?

Also, as I'm working on adding language to describe how to set up the --extra-vars YAML file, I realize that configuration will need to be set new variables into lms.env.json. For example: There is currently not support to automatically insert EDX_FIGURES into lms.env.json. This means that folks in the community will need to

A) use our configuration fork
B) Modify their instance (or edx.orgs's instance) of configuration
C) Update lms.env.json directly

Also, to note, edx-figures vars are loaded from lms.env.json to django.conf.settings via our envs/settings customizations. So implementors will need to customize their edx-platform envs/settings to use edx-figures I'm considering this to be beyond the scope for this initial PR. I don't want to get sidetracked/delayed. What I've done is create an issue item: #2

@johnbaldwin the standard edX way of adding new variables to lms.env.json is:

EDXAPP_LMS_ENV_EXTRA

So we shouldn't modify our fork at all.

A question: is server-vars.yml a common accepted community term for files the playbook includes via --extra-vars?

Not sure, you can ignore my comment about that.

I think @johnbaldwin is right, in most places in the docs they mention editing the lms.env.json file, probably because most people developing and testing things have devstack set up and that's the way to do it there, and if you're actually deploying production edX instances, you know how to get the variable to the generated json file anyway.

OmarIthawi · 2017-12-08T04:50:50Z

README.rst

+
+    "APPSEMBLER_FEATURES": {
+        "ENABLE_EDX_FIGURES": true,
+        "LMS_URLS_INCLUDE": [


Seems like a very easy contribution to edX, no? Added it to the list.

@OmarIthawi Yes, pretty easy. What testing do we need to include with this to help with the upstream PR? Also, please see my previous comment ^^^ . I think the vendor community around Open edX would find 'vendor additions' support in configuration and edx-platform to be helpful in reducing fork customization

@johnbaldwin not sure if it's worth any tests. You can go ahead and open the PR, I can help if edX has any comments. It would be a great exercise for us.

OmarIthawi · 2017-12-08T04:55:58Z

edx_figures/settings.py

+APP_DIR = os.path.dirname(os.path.abspath(__file__))
+
+# Mapping to create settings root level keys
+ENV_KEYS = [


Not sure why this is here, but please remember that edX also have a YAML config file. edx-figures should be use from django.conf import settings to consume either one transparently.

This is an artifact of me figuring out how to minimize edx-platform tailoring for custom packages like edx-figures. I don't need it now and can remove it.

@OmarIthawi I'm interested in how you would implement the following:

A) provide defaults in the reusable Django app
B) Ability to override those defaults with implementor custom settings

details:

A reusable django/open edx app (like edx-figures) has some default settings we declare in the package (in settings.py). If the implementor does not want to override these settings, the implementor doesn't have to do anything, just import these into django.conf.settings like we do in lms/envs/appsembler.py and lms/envs/(aws|devstack)_appsembler.py

If the implementor DOES want to override settings, then those settings are (ideally) generated via configuration into lms.env.json or (less ideally) manually added to lms.env.json. In either case we've got a scenario of being able to override defaults in the edx-figures package

OmarIthawi

Good start! I've added a big chunk for comments.

OmarIthawi · 2017-12-08T04:56:14Z

README.rst

+
+	from edx_figures.settings import EDX_FIGURES
+
+	EDX_FIGURES.update(ENV_TOKENS.get('EDX_FIGURES', {}))


Seems like an anti pattern to me! Let's make it work for edx/edx-platform smoothly. More comments in the settings.py file.

Also, just a few lines it's named EDX_FIGURES_SETTINGS which can cause confusion.

OmarIthawi · 2017-12-08T05:02:47Z

edx_figures/settings.py

+    }
+]
+
+EDX_FIGURES = {


Since this is expected to be overridden let's rewrite it like the following:

Import-time rewrite:
Add the following line after the definition:

EDX_FIGURES.update(ENV_TOKENS.get('EDX_FIGURES', {}))

Possibly the previous suggestion won't work because of circular dependency, here's a lazily loaded alternative:

def get_edx_figures_config(): configs = { .... configs.update(ENV_TOKENS.get('EDX_FIGURES', {})) return configs

Very clean, and compatible with vanilla edX (<-- should be a top priority since we're open sourcing it).

@OmarIthawi I'm not 100% clear on what you're suggesting. How about I merge this PR and you get this cleaner setttings approach to work in your sandbox and pull a PR with what you suggest for improving the custom settings? We can then document this approach together and use it as a model for future reusable django apps/open edx "plugin" apps

Agree with Omar, seems like a nice solution, just needs some testing as John mentioned.

OmarIthawi · 2017-12-08T05:03:56Z

edx_figures/tests.py

@@ -0,0 +1,3 @@
+from django.test import TestCase


OmarIthawi · 2017-12-08T05:04:43Z

edx_figures/urls.py

+from . import views
+
+urlpatterns = [
+    url(r'$', views.edx_figures_spa, name='edx-figures-spa'),


edx_figures_spa -> edx_figures_home (I like that more)

Sure, I have no issue renaming this

OmarIthawi · 2017-12-08T05:05:05Z

edx_figures/views.py

+def edx_figures_spa(request):
+
+    context = {
+        'edx_figures_api_url': 'http://api.foo.com',


Should be configurable, no?

@OmarIthawi Sure. This is a placeholder. As we build out the real API and figure out our configuration/settings organization, we'll load it from settings

OmarIthawi · 2017-12-08T05:06:27Z

setup.py

+    packages=find_packages(),
+    include_package_data=True,
+    license='MIT License',  # example license
+    description='Reporting and data retrieval for Open edX',


It's better if both package.json and setup.py are consistent. description is different.

Fixed. Consistent now, thanks!

OmarIthawi · 2017-12-08T05:07:51Z

package.json

+    "url": "git+https://github.com/appsembler/edx-figures.git"
+  },
+  "author": "",
+  "license": "ISC",


I hate it when npm suggests ISC license. Let's use MIT, well known permissive license. Unless of course you have another opinion.

Adding MIT for now. We can review which to put when we prep for opening up the repo

OmarIthawi · 2017-12-08T05:08:12Z

setup.py

+    version='0.1.0',
+    packages=find_packages(),
+    include_package_data=True,
+    license='MIT License',  # example license


Remove # example license

OmarIthawi · 2017-12-08T05:08:57Z

setup.py

+    classifiers=[
+        'Environment :: Web Environment',
+        'Framework :: Django',
+        'Framework :: Django :: X.Y',  # replace "X.Y" as appropriate


Let's do that # replace "X.Y" as appropriate comment and the comments below.

I set to 1.8 since that is what is used by Ficus

OmarIthawi · 2017-12-08T05:09:44Z

setup.py

+        'Topic :: Internet :: WWW/HTTP',
+        'Topic :: Internet :: WWW/HTTP :: Dynamic Content',
+    ],
+)


It's a bit annoying to see the X marks here for missing new lines. I'm perfectionist, I know!

Added newline

OmarIthawi · 2017-12-08T05:13:59Z

@johnbaldwin my comment looked more aggressive than I meant. Don't worry I'm not a Webpack hater 😄

I've added a lot of inline comments and here are high-level ones:

Lets get all the static files (CSS and JS) compiled and checked-in to the repo. Other users should be able to install this as an extra pip requirement that just works. This may seems like an anti-pattern, but edx-ora2 does that and it's for the greater good of re-usability!
Lets add some tests for integrating with Open edX, main ones are below. Those tests should help updating this repo from one Open edX release to another:
- Making sure migration works. Just in case someone at edX decided to use the same table names, or more piratically to drop a table/function we rely on.
- Ensure a sanity check or two about the modules/tables we use from edX.
Feel free to re-use any existing edX utils and classes, I'd even go ahead and recommend that. Try as much as possible to avoid direct database access, again, to help in upgrades.

johnbaldwin · 2017-12-11T01:12:39Z

@OmarIthawi Replies to top level comments:

Lets get all the static files (CSS and JS) compiled and checked-in to the repo.

After we have an actual working UI (that does something) +100%. Until then, I really don't think it makes sense. To note: I include compiled assets in Taxoman for exactly the reason you specified

Lets add some tests for integrating with Open edX

Since you've got significant experience developing tests for Open edx and I've got to get up to speed on this, I'd really appreciate what specific help you can provide that would either set a testing scaffolding for the different aspects we want to test or help me ramp up faster (or both).

Feel free to re-use any existing edX utils and classes,

Any suggestions here on something we've reinvented the wheel on in edx-figures?

Try as much as possible to avoid direct database access, again, to help in upgrades

Do you see any direct database access in this initial PR?

OmarIthawi · 2017-12-11T09:13:42Z

@johnbaldwin

To note: I include compiled assets in Taxoman for exactly the reason you specified

👍

Since you've got significant experience developing tests for Open edx and I've got to get up to speed on this, I'd really appreciate what specific help you can provide that would either set a testing scaffolding for the different aspects we want to test or help me ramp up faster (or both).

I'll take some time this week to initialize the the testing scaffolding.

Any suggestions here on something we've reinvented the wheel on in edx-figures? Do you see any direct database access in this initial PR?

Nope, just a future note.

OmarIthawi · 2017-12-11T09:22:00Z

@johnbaldwin Thanks for addressing the comments. I'll go ahead and approve the PR. I've added to issues that should help to keep the scope of this PR as is, however, suggests next-step improvements:

Support installation on vanilla edx/edx-platform and configuration #3: Support installation on vanilla edx/edx-platform and configuration
Add basic integration tests with edx/edx-platform #4: Add basic integration tests with edx/edx-platform

I will probably take the lead on #3 since providing the code would be easier than conveying idea. You're welcome to take the lead on #4, and I will be helping on that front as well.

bezidejni · 2017-12-11T10:39:00Z

README.rst

+
+1. Install edx-figures. ``pip install edx-figures``
+
+2. Add the following to ``lms.env.json``::


I think @johnbaldwin is right, in most places in the docs they mention editing the lms.env.json file, probably because most people developing and testing things have devstack set up and that's the way to do it there, and if you're actually deploying production edX instances, you know how to get the variable to the generated json file anyway.

bezidejni · 2017-12-11T10:40:18Z

README.rst

+Front-end Client App
+--------------------
+
+edx-figures has a skelton React app and simple/minimal webpack build configuration.


typo in word "skeleton"

bezidejni · 2017-12-11T10:42:24Z

README.rst

+
+	from edx_figures.settings import EDX_FIGURES
+
+	EDX_FIGURES.update(ENV_TOKENS.get('EDX_FIGURES', {}))


Also, just a few lines it's named EDX_FIGURES_SETTINGS which can cause confusion.

bezidejni · 2017-12-11T10:44:46Z

edx_figures/settings.py

+    }
+]
+
+EDX_FIGURES = {


Agree with Omar, seems like a nice solution, just needs some testing as John mentioned.

OmarIthawi · 2017-12-11T14:02:51Z

@johnbaldwin could you please add "write access" to me?

Please see: #1 NOTE: This is a work in progress (WIP) with the next stage to incorporate UI work that Matej is working on. So the asset management is going to likely change I pulled out installation and configuration instructions from the README since there were significant comments in the PR about this and this is definitely going to change as we work to make this package easier to intergrate with edx-platform.

johnbaldwin · 2018-01-18T18:08:39Z

@OmarIthawi I addressed a number of your comments. To note: How edx-figures is configured and installed in edx-platform is likely going to change, so instead of putting a lot of time into making solid installation and configuration instructions, I scrubbed that out of the README. As Matej and I work on the package and we figure out how we're going to structure asset management I'll update the README when we know where things are going

Matej needs this repo now in order to incorporate the UI work he's done so far, so unless you see anything that is fundamentally breaking, please approve.

Thanks!
-John

johnbaldwin · 2018-01-18T18:11:20Z

@OmarIthawi Openedx team had read only permissions, sorry. Changed to read/write

OmarIthawi · 2018-01-18T18:35:52Z

Thanks @johnbaldwin! Let's move this forward and address the issues in future PRs.

johnbaldwin · 2018-01-18T18:40:05Z

@OmarIthawi Thanks! Also thanks for noting things in the issues section, I appreciate it!

Bump figures to 0.3.12

johnbaldwin added 2 commits November 10, 2017 01:21

WIP getting webpack and django-webpack-loader working

f19db0b

johnbaldwin requested review from tkeemon, OmarIthawi, melvinsoft, bryanlandia and amirtds December 4, 2017 16:56

Baseline React skeleton app

0981f80

johnbaldwin assigned grozdanowski Dec 4, 2017

OmarIthawi reviewed Dec 8, 2017

View reviewed changes

OmarIthawi suggested changes Dec 8, 2017

View reviewed changes

OmarIthawi mentioned this pull request Dec 11, 2017

Add installation instructions #2

Closed

OmarIthawi approved these changes Dec 11, 2017

View reviewed changes

bezidejni reviewed Dec 11, 2017

View reviewed changes

johnbaldwin merged commit 9d3373b into develop Jan 18, 2018

johnbaldwin deleted the initial_baseline_structure branch January 18, 2018 18:40

johnbaldwin mentioned this pull request Jul 1, 2020

New endpoint for enrollment metrics - provide data captured in LCGM #233

Merged

johnbaldwin pushed a commit to johnbaldwin/figures that referenced this pull request Sep 29, 2020

Merge pull request appsembler#1 from appsembler/master

819c6a5

Bump figures to 0.3.12

johnbaldwin mentioned this pull request Apr 27, 2021

Pytest is missing devsite .env file and default Open edX release #331

Closed


		1. Install edx-figures. ``pip install edx-figures``

		2. Add the following to ``lms.env.json``::


		from edx_figures.settings import EDX_FIGURES

		EDX_FIGURES.update(ENV_TOKENS.get('EDX_FIGURES', {}))

Initial commit of Django application boilerplate and base models #1

Initial commit of Django application boilerplate and base models #1

Conversation

johnbaldwin commented Dec 4, 2017 • edited Loading

Some details:

johnbaldwin commented Dec 4, 2017

OmarIthawi commented Dec 5, 2017 • edited Loading

grozdanowski commented Dec 7, 2017

johnbaldwin commented Dec 7, 2017

johnbaldwin commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarIthawi Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

details:

OmarIthawi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarIthawi commented Dec 8, 2017 • edited Loading

johnbaldwin commented Dec 11, 2017 • edited Loading

OmarIthawi commented Dec 11, 2017

OmarIthawi commented Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarIthawi commented Dec 11, 2017

johnbaldwin commented Jan 18, 2018

johnbaldwin commented Jan 18, 2018

OmarIthawi commented Jan 18, 2018

johnbaldwin commented Jan 18, 2018

johnbaldwin commented Dec 4, 2017 •

edited

Loading

OmarIthawi commented Dec 5, 2017 •

edited

Loading

OmarIthawi Dec 8, 2017 •

edited

Loading

OmarIthawi commented Dec 8, 2017 •

edited

Loading

johnbaldwin commented Dec 11, 2017 •

edited

Loading

OmarIthawi commented Dec 11, 2017 •

edited

Loading