Feature/cdodge/alpha microsite2 #2061
Conversation
|
|
||
| # add to the existing maps in our settings | ||
| subdomain_branding[domain] = university | ||
| virtual_universities.append(university) |
There was a problem hiding this comment.
It looks like virtual_universities and subdomain_branding are getting set here, but never used?
There was a problem hiding this comment.
Yea, that was some legacy settings that I wasn't 100% sure if was safe to remove, so I just added on to it. I'm not sure if the MITx folks use that or not. I'll check in with Ike's team.
There was a problem hiding this comment.
@carsongee Do you have any insight into whether MIT is using that feature?
There was a problem hiding this comment.
We are still using it for some one off use cases to limit what classes show for a given service variant and would prefer to keep it available, but I haven't looked closely enough at this PR to see if this would work in replacing that use case with something better. We really just want to have a URL or service variant that shows a subset of classes (definable in settings or by course org). We don't currently use sub theming for that use case, but it could be nice.
|
@chrisndodge -- @caesar2164 and I looked at your branch a bit today and it looks sane and an improvement over the previous approach. We didn't have time to do an actual code review, sorry to say. Nor were we able to actually try the feature itself. We could use and example of how to call In addition to documentation on how to actually turn this on, it'd be nice to think about how to support dev/test. I don't want to fiddle with my hosts file to give myself a subdomain. Ideally there'd be some way to force a subdomain easily (query parameter?). Or maybe it's in there already and I just need a pointer to it. If you send instructions for configuration and forcing the domain I'm happy to take more of a look tomorrow. But this is a good start. Thanks Chris. |
|
@sefk thanks for the initial feedback. I will need to move on to a more formal code review pretty soon (I was thinking of including Dave O) as this will need to be merged in no later than this coming Monday. Also I met with @jarv this afternoon and he wants to look at this PR as well. I'll forward a sample lms/envs/xxxx.py configuration file tomorrow (sorry, I left my laptop at the office by accident) to better illustrate the usage (or maybe I'll just check it in). I'll also add some in-code comments as well as follow up if we can formally deprecate the SUBDOMAIN_BRANDING and VIRTUAL_UNIVERSITY constructs in the code. I'll be meeting with QA tomorrow re: testing. Using querystrings (or maybe even a cookie) might be a good way to get around the hostname configurations for automating testing - thanks for the suggestion. |
|
@chrisndodge I'm also concerned about the lack of documentation for the settings. I think this is in the same ballpark as the enable marketing site settings that I had to circle back around to and document, which I'm sure took way longer than if we had done it as we went. Maybe you could put something together like that wiki page? https://github.com/edx/edx-platform/wiki/Alternate-site-for-marketing-links |
| _microsite_configuration_threadlocal.data = microsite_configuration | ||
|
|
||
| # also put the configuration on the request itself to make it easier to dereference | ||
| request.microsite_configuration = _microsite_configuration_threadlocal.data |
There was a problem hiding this comment.
Why not add it to the session? Why can't one of these mechanisms for passing this around be sufficient?
There was a problem hiding this comment.
It doesn't look like request.microsite_configuration is used?
|
Some high level questions and comments.
|
|
|
||
| try: | ||
| with open(microsite_root / "config.json") as microsite_config_file: | ||
| microsite_config = json.load(microsite_config_file) |
There was a problem hiding this comment.
@chrisndodge are we sure there won't be a later requirement to show courses from multiple orgs on a microsite landing page?
I would really like to keep the configuration outside of the microsite_root or put this configuration in the database. If we are filtering by org I don't see this configuration changing very often so maybe the best shortterm thing to do is to move it to env.json?
|
@chrisndodge, I know we briefly went over your proposal in person before the holidays. Do you need me or my team to review this work formally (I think this was noted as a next step last we left things) from a Front End (HTML, CSS/Sass) perspective before merging? |
|
@mattdrayer just looping you in on the Microsite pull request in case you are interested in following along |
|
@chrisndodge - I know you all are reviewing this from an "internal" (MIT, On Tue, Jan 7, 2014 at 7:11 PM, chrisndodge notifications@github.comwrote:
|
|
@yarko Thanks for the input. If you really want to try it out I'll need to add some documentation to the PR on how to set up. I'd be happy to get feedback, but please note that we have a very hard (and very soon) deadline. So there might have to be additional iterations to accommodate feedback. The aim here is to allow of branding themes on a per sub domain basis. It does not allow for full multi tenancy, eg partitioned user database. Thanks again for you interest! Sent from my iPhone On Jan 7, 2014, at 8:38 PM, yarko notifications@github.com wrote:
|
|
@yarko I have to respond to our DevOps team to request to change some of the configuration management. I'd advise that you wait a day to give this branch a spin as I have to check in a few more changes, plus it will change how to set this up for you to test. |
|
Yep - pretty aware of all that which is why I gave heard up. After your
|
|
@chrisndodge -Just read - will wait to Fri, or after this merge if it turns
|
| def process_request(self, request): | ||
| """ | ||
| Middleware entry point on every request processing. This will associate a request's domain name | ||
| with a 'Univserity' and any corresponding microsite configuration information |
|
The API for this feature is very awkward, and I suspect it could be greatly simplified. @chrisndodge, can we talk about what this feature needs to do, and define a nicer API? |
| @@ -1065,6 +1067,61 @@ def enable_theme(theme_name): | |||
| STATICFILES_DIRS.append((u'themes/%s' % theme_name, | |||
| theme_root / 'static')) | |||
|
|
|||
|
|
|||
| ############################### MICROSITES ################################ | |||
| def enable_microsites(microsite_config_dict, subdomain_branding, virtual_universities, microsites_root = ENV_ROOT / "microsites"): | |||
There was a problem hiding this comment.
Why is this function defined in a settings file? It seems like it should be defined in the microsites app, and invoked as part of the startup?
|
Thanks @nedbat. I'll go through and address couple of these tomorrow. Some of these I think are purely stylistic and not terribly urgent, in my opinion, as I need to focus some time on integration tests and clean up the known pep8/pyline violations. |
| # HTTP request header from the rest of the Django middleware pipeline | ||
| # then we're OK. Need to investigate a bit more. | ||
| if domain == 'test_microsite.testserver': | ||
| del request.META['HTTP_HOST'] |
There was a problem hiding this comment.
Using "@override_settings" might solve this issue (see my comment in the test module). In any event, it makes me really uncomfortable to add a special case for the test suite -- we shouldn't need to do this.
|
@wedaly the 'secret' to why those SuspiciousOperations were happening can be found at: http://timmyomahony.com/blog/suspiciousoperation-invalid-http-host-header-django/ If I remove the underscore from the test hostname, it works fine. |
|
@nedbat addressed many of your comments. The ones which involve a rename or a refactoring (e.g. separate Middleware from static accessors) I'm deferring since they will require a lot of touch points. Can I get a +1 on the changes and then I'll do the rebase. To keep on schedule (release needs to be done on 1/21) this needs to be merged today. Thanks |
|
@chrisndodge In no way do I want to hold up delivery of this, so go ahead and merge it. But just to be clear, my biggest concerns are not yet addressed: modification of settings in process_request, using one class as both Middleware and the configuration object, and a new 50-line function in settings.py. These really have to be fixed. |
|
Also, there are still three skipped tests. |
|
Spoke with @wedaly and @dmitchell. Will resolve this merge conflict (looks trivial by eye) on merging of Release back to Master. |
…tenant branding on a subdomain basis, e.g. foo.edx.org and bar.edx.org fix errorenous logic when running a microsite that could reside in a hosting environment with a marketing site in front of it pep8/pylint fixes address PR feedback, remove underscore from test hostname more pep8/pylint cleanup. Skip test_microsites test, it works on localdev, not on Jenkins. Need to talk with QA team manually add Ned's single-to-double quote fix change aws.py runtimes so that the microsite_dir that is read from configuration is changed to a python path
…rositeConfigurations will override the global setting one more faulty login regarding ENABLE_MKTG_SITE=true situations
|
@chrisndodge - is this ready for an "outsider" to try, give commentary on? (anything I need to grab other than your branch?) I'm frankly more interested in full theming at this point, but elements and their interactions all fall into that view for me, now. |
|
We're doing final QA on this branch as it's planning to be release early On Thu, Jan 16, 2014 at 5:45 PM, yarko notifications@github.com wrote:
|
| VIRTUAL_UNIVERSITIES = ENV_TOKENS.get('VIRTUAL_UNIVERSITIES', []) | ||
|
|
||
| MICROSITE_CONFIGURATION = ENV_TOKENS.get('MICROSITE_CONFIGURATION', {}) | ||
| MICROSITE_ROOT_DIR = ENV_TOKENS.get('MICROSITE_ROOT_DIR') |
There was a problem hiding this comment.
@chrisndodge - I believe this should have a default value, otherwise it could throw a key error if the configuration script does not define a value for this key.
|
Note: this PR has been merged to master (cherry picked). Keeping this PR open to be able to address comments/suggestions through follow-up commits. |
| @@ -1048,6 +1050,64 @@ | |||
| } | |||
|
|
|||
|
|
|||
| ############################### MICROSITES ################################ | |||
| def enable_microsites(microsite_config_dict, subdomain_branding, virtual_universities, microsites_root=ENV_ROOT / "microsites"): | |||
There was a problem hiding this comment.
This method should be moved from a settings.py file into the microsites app itself.
There was a problem hiding this comment.
Yea, I followed what Stanford did. However, since then, someone moved that code (to startup.py?!?). I'll do the same.
|
I went through the code and put up some doc for your review here -> https://github.com/edx/edx-platform/wiki/Microsites-Theming Comments, edits, and corrections welcome! |
|
Thanks, I'll add steps on how to set up (e.g. which folders to add for microsite content, etc.). |
|
|
||
| <% | ||
| if self.theme_enabled(): | ||
| google_analytics_file = u'../' + MicrositeConfiguration.get_microsite_configuration_value('google_analytics_file', 'theme-google-analytics.html') |
There was a problem hiding this comment.
I believe this should be in the else block, not the if block?
Move microsite test assets to common/test Rename helper method to not have test in the name Rename test css file
| @@ -523,7 +533,10 @@ def refund_cert_callback(sender, course_enrollment=None, **kwargs): | |||
| user_email=course_enrollment.user.email, | |||
| order_number=order_number) | |||
| to_email = [settings.PAYMENT_SUPPORT_EMAIL] | |||
There was a problem hiding this comment.
This should be wrapped also.
As well as other occurrences of PAYMENT_SUPPORT_EMAIL in the methods for this class. E.g. additional_instruction_text
|
Thanks - I'll have a look... On Wed, Jan 22, 2014 at 2:23 PM, Jay Zoldak notifications@github.comwrote:
|
|
Closing this PR. It is being picked up by #2553 |
* MCKIN-28999: bump api-integration version * bump CI cache version
@sefk would you be in a position to give this a quick once over? I still have some cleanup to do and tests to write, but I think this addresses some of your feedback (i.e. having template/.css overrides in a directory outside of edx-platform). I still need to chat with DevOps about how these microsite definition directories get deployed/managed.
Ideally, I need to have the approach locked down next week as the customer should start delivering some of the override assets.
UPDATE: @ichuang can I get some MITx eyeballs on this as well? Basically this feature is to support separate deployable "micro-themes" (e.g. branding elements, Mako template overrides, etc.) so that we can get a bit of branding "multi-tenancy" on the edx.org production servers (e.g. foo.edx.org, bar.edx.org). My primary concern is regarding preventing regressions with MITx (and Stanford who is looking at this now) instances. Question for your team: Does the MITx instance actually use the legacy SUBDOMAIN_BRANDING and VIRTUAL_UNIVERSITIES settings? Sef @Stanford was asking if - it's not actually used - if we can get rid of it. Thanks.