Address Microsite PR feedback - separate out middleware from microsite logic, shor... #2553

chrisndodge · 2014-02-11T03:20:09Z

...ten up names, etc.

chrisndodge · 2014-02-11T05:34:42Z

@nedbat @jzoldak @singingwolfboy - I believe this addresses your remaining concerns on the original PR.

@sefk just cc:'ing you in as well in case you want to do a regression test

I need to do some more manually testing, but I thought I'd get the code review started.

chrisndodge · 2014-02-11T06:20:20Z

@antoviaque you'd be interested in this as well since we are using Microsites for some of our projects. I think the only configuration delta here is that we need to now set a FEATURES['USE_MICROSITES'] = True.

Worst case, I can grandfather in not setting this feature flag and inferring it from if one had set the MICROSITE_CONFIGURATION configuration dictionary.

davestgermain · 2014-02-11T14:21:30Z

common/djangoapps/microsite_configuration/microsite.py

+_microsite_configuration_threadlocal.data = {}
+
+
+class Microsite(object):


If you have a class that's never instantiated and consists entirely of classmethods, why not just make them functions at the module level?

That might be the .NET coder in me coming out :-)

I'm happy to change it, but - stylistically speaking - I do like to do this kind of "namespacing". Is there something particularly "un-pythonic" about that pattern?

Namespacing can and should be done using files/modules, not with classes. If a function can be decoupled from a class definition, it should be -- attaching it to a class adds unnecessary complexity, and often makes unit testing more difficult.

Thanks.

well, unfortunately, I'm getting conflicting feedback on this feature. On one hand I've been asked to make the function names shorter (from @nedbat) , e.g. get_microsite_configuration_value() -> get_value(). However, if I make these straight up classless methods, then those names become pretty ambiguous and so the longer name (get_microsite_configuration_value()) makes more sense.

The module already provides a namespace: microsite.get_value() You can import microsite rather than Microsite.

ah, gotcha.

Thanks

I agree -- if you decouple the functions from the class, then the longer names make sense. At the same time, I think that you could dramatically simplify the data model, and simply expose the microsite configuration dict, so that people could import that and use standard Python indexing syntax (or standard dict methods like get()) to achieve the same result, which could make sense as well. Check out the other pull request I did for this feature to get a feel for what I'm referring to.

chrisndodge · 2014-02-11T21:14:29Z

Thanks for the feedback. It might take a day or two to address.

antoviaque · 2014-02-12T01:11:46Z

common/djangoapps/microsite_configuration/microsite.py

+
+        for key in settings.MICROSITE_CONFIGURATION.keys():
+            subdomain = settings.MICROSITE_CONFIGURATION[key]['domain_prefix']
+            if domain.startswith(subdomain):


Could you add support for a 'default' configuration, which would apply a given microsite configuration when it doesn't match any subdomain? It would make it easier to use it to skin instance with only one skin, and would allow support for domains without a subdomain, like edx.org.

antoviaque · 2014-02-12T01:13:46Z

@chrisndodge Sounds good to add FEATURES['USE_MICROSITES'] -- that's more consistent with how other optional features are activated, so good for me.

chrisndodge · 2014-02-13T19:43:05Z

@nedbat @davestgermain @singingwolfboy @antoviaque - responded to your feedback. Waiting for build results.

One note, we still have a "hack" for detecting when on Edge and showing a different homepage. The only other approach I could think of is make Edge be a "microsite" just in terms of the subdomain matching (but not define any overrides) but that would require config changes to be pushed through as well otherwise we'd loose the Edge homepage.

@sefk keeping you in the loop in case you all want to do any regression tests or have any other feedback.

chrisndodge · 2014-02-18T18:16:22Z

This PR replaces #2061

singingwolfboy · 2014-02-18T18:56:39Z

cms/envs/common.py

@@ -76,6 +76,9 @@

    # Allow editing of short description in course settings in cms
    'EDITABLE_SHORT_DESCRIPTION': True,
+
+    # Turn on/off Microsites feature
+    'USE_MICROSITES': False,


Nitpick: I'd prefer to refer to settings.FEATURES['MICROSITES'] rather than settings.FEATURES['USE_MICROSITES']. It's not a big deal, though -- I think "USE" is unnecessary, but if you think the "USE" makes things clearer somehow, then I'm OK with it.

chrisndodge · 2014-02-25T06:05:47Z

@sefk @jbau do you want to take a pass on this as well? I have to get this live in next weeks (3/5) push for a customer. Basically it's the same as the original implementation, but with some code refactoring.

chrisndodge · 2014-02-25T23:57:49Z

Sorry @jbau @sefk just wanted to ping you again as I need to get this merged in the next day or two in order to meet a client launch. Thanks.

jbau · 2014-02-26T00:02:21Z

I'll try to review tonight, tomorrow morning: 1st high level comment, can I get you to incorporate
https://github.com/edx/edx-platform/pull/2534/files
into this PR? Just 2 tiny changes.

chrisndodge · 2014-02-26T00:31:47Z

@jbau done. And of course there's now a merge conflict on this PR :-)

jbau · 2014-02-26T19:07:35Z

@chrisndodge well, at least tests passed =)
reviewing now

jbau · 2014-02-26T19:37:34Z

OIC. You merged #2534 into master. I had thought you would merged it into this PR, but no matter.

jbau · 2014-02-26T19:45:29Z

OK. This looks fine to me, once the merge conflict and other comments are resolved.

chrisndodge · 2014-02-26T19:48:42Z

OK, will rebase and squash commits. I'll try to squeeze in @nedbat and @singingwolfboy's small comment.

jzoldak · 2014-02-27T15:45:24Z

common/djangoapps/microsite_configuration/tests/tests.py

@@ -0,0 +1,26 @@
+"""


Please rename this to something like: test_logic.py, and move ../test_microsites.py file into the tests directory also.

I'd like to move test_microsites.py but it's dependent on LMS serving up HTML, so shouldn't it remain in lms tests?

chrisndodge · 2014-02-27T21:26:13Z

@singingwolfboy @nedbat @antoviaque care to do another final pass? I'd like to merge this in tomorrow AM.

chrisndodge · 2014-02-27T21:26:24Z

@jzoldak ^

antoviaque · 2014-02-27T21:54:30Z

cms/envs/test.py

+        "email_from_address": "test_microsite@edx.org",
+        "payment_support_email": "test_microsite@edx.org",
+        "ENABLE_MKTG_SITE": False,
+        "SITE_NAME": "test_microsite.localhost",


That made me realize that different options have different cases - I understand where this comes from, and this is not a blocker, but while we are doing changes, it might be worth capitalizing all options for consistency.

antoviaque · 2014-02-27T22:10:01Z

lms/envs/test.py

-        microsites_root=COMMON_ROOT / "test" / 'test_microsites'
-    )
+MICROSITE_ROOT_DIR = COMMON_ROOT / 'test' / 'test_microsites'
+FEATURES['USE_MICROSITES'] = True


That should be enabled by your tests during setup instead of overriding the default for all tests, no?

I thought it'd be good to be global to make sure that we exercise non-microsite renderings through the existing tests. If we localize this FEATURE flag to just the microsite tests, then we don't get test execution which simulate what it'll be like when we have this FEATURE flag set in product, but the requests don't actually go through them.

OK, to skip this recommendation?

antoviaque · 2014-02-27T22:16:54Z

Besides the last two remarks, LGTM. A lot of cleanup, thanks for this!

singingwolfboy · 2014-02-27T23:49:35Z

common/djangoapps/microsite_configuration/tests/test_logic.py

+
+        # now test when we call in a value Microsite ORG, note this is defined in test.py configuration
+        value = get_value_for_org("TestMicrositeX", "university", "default_value")
+        self.assertEquals(value, "test_microsite")


Please also write tests that cover (at a minimum) get_template_path, get_all_orgs, get_value_for_org, and set_by_domain. It would also be nice if you could write a test that works more at the integration level: use the Django test client to get the HTML for a page that should change depending on the active microsite, assert that some microsite-specific things are present in that HTML, assert that some things that they replace are not present, and assert that some things specific to a different microsite in the configuration are not present. Also assert that you can hit that same page without the microsite prefix in the URL, and that the original page is returned, without the microsite customizations.

We do have integration level test which pass in a microsite domain and inspects the HTML output. It's in lms/djangoapps/courseware/test_microsite.py, or something like that - I don't have the source code in front of me right now.

Ah, OK. Never mind, then. :)

chrisndodge · 2014-02-28T14:25:56Z

@singingwolfboy addressed a number of your comments. With regards to test coverage, we have coverage on a number of your suggestions via the LMS integration tests. I believe we are at 95% coverage right now for this PR

@jzoldak @antoviaque @nedbat

Can I get two thumbs up?

singingwolfboy · 2014-02-28T14:49:22Z

lms/templates/courseware/course_about.html

@@ -20,13 +20,15 @@

  <%
    if self.theme_enabled():
-      google_analytics_file = u'../' + MicrositeConfiguration.get_microsite_configuration_value('google_analytics_file', 'theme-google-analytics.html')
+      google_analytics_file = u'../{ga}'.format(
+        ga = microsite.get_value('google_analytics_file', 'theme-google-analytics.html')


Nitpick: pep8 doesn't like the whitespace on either side of the =, because it's for keyword arguments to a function. Looks like the Diff Quality tool isn't catching it because this is Python embedded in a Mako template.

singingwolfboy · 2014-02-28T14:55:20Z

Still needs some comments around calling edxmako.startup.run multiple times. Once you've added those, 👍

Thanks for doing this!

chrisndodge · 2014-02-28T17:58:45Z

@jbau can I get thumbs up from you as well as the "second +1"? You implicitly signed off up above, but it'd be nice to formalize it. Thank you.

jzoldak · 2014-02-28T20:53:41Z

👍

jbau · 2014-02-28T21:14:39Z

👍, but rebase first =)

chrisndodge · 2014-02-28T22:18:01Z

@antoviaque this is getting merged now. This is just a reminder that we need to add the FEATURE flag to the any Customer-specific configuration deploys. Thank you.

Address Microsite PR feedback - separate out middleware from microsite logic, shor...

Release 2018-2Q (201804-201806)

davestgermain reviewed Feb 11, 2014
View reviewed changes

antoviaque reviewed Feb 12, 2014
View reviewed changes

chrisndodge mentioned this pull request Feb 18, 2014

Feature/cdodge/alpha microsite2 #2061

Closed

singingwolfboy reviewed Feb 18, 2014
View reviewed changes

jzoldak reviewed Feb 27, 2014
View reviewed changes

antoviaque reviewed Feb 27, 2014
View reviewed changes

singingwolfboy reviewed Feb 27, 2014
View reviewed changes

singingwolfboy reviewed Feb 28, 2014
View reviewed changes

This PR addresses the open feedback on the Microsites feature

aa43689

chrisndodge pushed a commit that referenced this pull request Feb 28, 2014

Merge pull request #2553 from edx/cdodge/microsite-cleanup

c4401fd

Address Microsite PR feedback - separate out middleware from microsite logic, shor...

chrisndodge merged commit c4401fd into master Feb 28, 2014

jzoldak deleted the cdodge/microsite-cleanup branch May 5, 2014 14:54

jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Jun 26, 2018

Merge pull request openedx#2553 from nttks/develop/dogwood/201806

a6b37cf

Release 2018-2Q (201804-201806)

		_microsite_configuration_threadlocal.data = {}


		class Microsite(object):

Address Microsite PR feedback - separate out middleware from microsite logic, shor... #2553

Address Microsite PR feedback - separate out middleware from microsite logic, shor... #2553

Conversation

chrisndodge commented Feb 11, 2014

chrisndodge commented Feb 11, 2014

chrisndodge commented Feb 11, 2014

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

chrisndodge commented Feb 11, 2014

Choose a reason for hiding this comment

antoviaque commented Feb 12, 2014

chrisndodge commented Feb 13, 2014

chrisndodge commented Feb 18, 2014

Choose a reason for hiding this comment

chrisndodge commented Feb 25, 2014

chrisndodge commented Feb 25, 2014

jbau commented Feb 26, 2014

chrisndodge commented Feb 26, 2014

jbau commented Feb 26, 2014

jbau commented Feb 26, 2014

jbau commented Feb 26, 2014

chrisndodge commented Feb 26, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisndodge commented Feb 27, 2014

chrisndodge commented Feb 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoviaque commented Feb 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisndodge commented Feb 28, 2014

Choose a reason for hiding this comment

singingwolfboy commented Feb 28, 2014

chrisndodge commented Feb 28, 2014

jzoldak commented Feb 28, 2014

jbau commented Feb 28, 2014

chrisndodge commented Feb 28, 2014