Ziafazal/wl-245: multiple backend support for microsite #11073
Conversation
|
jenkins run js |
|
jenkins run python |
|
@chrisndodge @felipemontoya -- here is an updated PR for #10158 -- it's currently WIP, but feel free to take a look when you have a moment. |
a133776 to
5787684
Compare
|
@mattdrayer we finally have a green build now. |
|
@chrisndodge @felipemontoya -- looking like we are in good shape now thanks to @ziafazal's hard work! If you can find some time in the next 24 hours to review this PR, I will also take a pass through and assuming everything looks good we can merge this week. |
|
@mattdrayer, @chrisndodge, @ziafazal. It's nice to see this in a green build. I just got back to the office, but I will sure take a good look at this today and report my findings |
|
@mattdrayer @ziafazal, I gave this a good look and it worked nicely. Great work @ziafazal. |
|
@felipemontoya now that we have merged your PR are you good with this changeset? |
|
As far as I can tell we are ready with this |
|
Okay, great, then I'm going to give it a 👍, and if @ziafazal can squash the commits maybe we can get @douglashall or @mjfrey to take a look at the code tomorrow. |
|
As far as I can tell we are ready with this, and I'm very good with the changeset |
d5eef5c to
6a024ef
Compare
|
@mattdrayer squashed commits. |
|
@mattdrayer code looks good. I don't have all the context for this. |
|
@mjfrey neither do I :) most of the context is still with @chrisndodge but we do have @felipemontoya as a resource and @ziafazal has gained knowledge through this PR as well. So we appreciate your review of the code even w/o all of the background. |
| """ | ||
| Overridden method which will allow us to look up templates from a database (optionally) | ||
| """ | ||
| from microsite_configuration.microsite import get_template as get_microsite_template |
There was a problem hiding this comment.
why is this import not in the file header and in a method?
|
would also be nice to document an example use of each backend. In a comment of each backend. |
6a024ef to
172eddf
Compare
|
Thanks @mjfrey. I have tried to answer questions and made changes as per your feedback. |
|
jenkins run bokchoy |
172eddf to
0b65596
Compare
|
jenkins run lettuce |
|
|
||
| # now test when we call in a value Microsite ORG, note this is defined in test.py configuration |
There was a problem hiding this comment.
It would be better to break this second check out into a separate test.
|
Okay @ziafazal I have completed my revisit -- hopefully I have not made too many requests! Mostly I think I am looking for some additional info/comments and some renames + light refactoring. However I did have one or two things that may require additional work -- please let me know what you think. |
cb6c628 to
8131a29
Compare
|
@mattdrayer I have made almost all of the changes you suggested including adding django sites framework support to |
|
Thanks @ziafazal, this was great work. I am downloading it to my devstack now |
|
jenkins run python |
|
@mattdrayer and @ziafazal , I tested this throught on my devstack and I found that during the rebase some of the modifications from #10623 were reverted. Specifically the As long as we don't have a dynamic template finder for the django template, the enable_microsite routine, will have to be split into a pre django.setup() and a post django.setup(). I'll make a PR against this branch with said correction. |
|
jenkins run python |
Adding the declaration of the settings object to openedx.conf to be able to import it from a nicer location Resolving quality violations Merging dicts with the settings definition when they exist in the microsite configuration Using a cache to improve the perfomance of quering any dictionary in the microsite definition Ignoring the invalid-name pylint warning since the names must be kept thsi way to stay the same as the ones in django. Removing the default dict argument as per https://docs.python.org/2/tutorial/controlflow.html#default-argument-values Extracting the implementation of the microsite to a selectable backend. Leaving the function startup.enable_microsites for backwards compatibilityy Adding a database backend Using a cache to improve the perfomance of quering any dictionary in the microsite definition. Changed the database backend so that it extends the settings file backend and removed all the unnecessary methods. Using the backend provider for the get_dict function some tweeks and some initial unit tests Using getattr as a function insteal of calling the underlying __getattr__ directly Adding an ModelAdmin object for the microsite model in the django-admin panel refactor enable_microsites() consolidate/refactor some shared code add config to aws.py and add migration files fix tests Changes to get the backends to run after the refactor add archiving capabilities to microsites. Also make a few notes about performance improvements to make fix tests Making the query to find if microsites exist in the database faster add ORG to microsite mapping tables and some performance improvements allow for Mako templates to be pulled from the database fix tests For the database template backend the uri of the template does not use the filesystem relative path Fixing pylint violations Added caching of the templates stored in the database Fixing pylint errors fix pylint Clearing the cache on model save Fixing pylint errors rebased and added test coverage rebased cdodge/microsite-improvements branch with master and added test coverage added missing migration fix quality violations add more test coverage mattdrayer: Add microsite_configuration to cms.INSTALLED_APPS added microsite settings to cms/envs/test.py run session cookie tests only in LMS fixed broken tests putting middleware changes back Preventing the template_backend to be called on requests which have no microsite changes to address feedback from mjfrey changed BaseMicrositeBackend to AbstractBaseMicrositeBackend changes after feedback from mattdrayer fixed broken tests and quality violations Allowing the backend to handle the enable_pre_startup routine Typos and docstrings Adressing feedback Fixing python tests add comment to explain why we need enable_microsites_pre_startup()
d757eab to
4742e66
Compare
|
Alright, this epic PR is looking good to me at this point -- @felipemontoya can you take one last look now that your changeset has been merged. FYI, the next RC will be cut today at 11am Eastern, so if we're good to go I will merge this morning. |
|
@mattdrayer I downloaded it again, fired it on devstack on some test scenarios, and went through the code again. It looks good to me 👍 |
|
Fantastic -- okay everyone, we are SHIPPING this changeset -- nice work all!!! 👍 👍 👍 |
Ziafazal/wl-245: multiple backend support for microsite
@mattdrayer This is PR has some enhancements to microsite module. e.g allows multiple backend support for microsites configuration and templates. I have added some test coverage up 95%.