Microsites alongside Theme

[felipemontoya]

The logic is to first check that the request is not a microsite-request before choosing the branding templates from the theme.

[singingwolfboy]

Thanks! @chrisndodge, can you take a look at this?

[chrisndodge]

By eye, looks about right. Can you explain a bit about the motivation here? Do you want a "Stanford theme" on the primary hostname and then a microsite on some other subdomains?

Thanks again for the contribution!

[sefk]

I pulled this branch and tried running in my dev environment and it did no harm (tm). Nice work! I'll try configuring a microsite per the instructions. (It'd be nice to work off an example if there is one somewhere).

[felipemontoya]

@chrisndodge The motivation here is to merge the theming and microsites ways of branding. There is a discussion on this here

@sefk I usually use the example under lms/envs/cms/microsites_test.py to test

[chrisndodge]

@antoviaque do you mind taking a quick look at this because, I think we have a case of a customer which is using both microsites and stanford theming at the same time, so I think this change:

+  if theme_enabled() and is_main_site():
      header_extra_file = 'theme-head-extra.html'
      header_file = 'theme-header.html'
      google_analytics_file = 'theme-google-analytics.html'

might have an adverse affect because - if I understand correctly - you are using these templates while at the same time using microsites for some additional branding.

[antoviaque]

From a quick look, the is_main_site() function checks if there is a microsite configuration for the current domain, and only returns True if there is none for the current domain, so we should be good. Imho it would be worth renaming that function to something like is_microsite() (and check for not is_microsite() as that would be less misleading.

[felipemontoya]

@antoviaque Thanks for the sugestion. It does make it clearer to read.

[sarina]

@felipemontoya This needs to be rebased to make sure it does not conflict with opaque-keys, and a test build needs to be run.

@chrisndodge After that, is more review work needed?

[sarina]

• @sefk - do you think this needs further review?

[singingwolfboy]

@felipemontoya I take it that this is no longer a "work in progress" pull request. If so, can you change the title to reflect that?

[felipemontoya]

@singingwolfboy I wanted use this to actually call Theme templates from the microsite function to get the template. But I have not gotten around to do so. In any case I can do so in a new pull request. I wil l change the name for now.

[cpennington]

This is wacky. is_request_in_microsite should just be returning bool(get_configuration()), and then you wouldn't have to check against {} explicitly in this code.

[felipemontoya]

I like it, is way better. I will see if this affects other parts of the codebase and change accordingly.

[sefk]

Aside from those good comments from @cpennington this doesn't need any more review from me. 👍

[sarina]

@felipemontoya can you address comments, rebase, and then post here when you've done that? I'll then run a build on your PR.

[felipemontoya]

@cpennington thanks for the feedback, I did the changes you suggested. There were no other places using is_request_in_microsite(only get template path, but it was using it in the same way), so there was no need to change anything else.

[sarina]

Running a build now - I'll let @cpennington decide whether or not to merge.

[cpennington]

👍

[cpennington]

@antoviaque @chrisndodge @felipemontoya would you guys be able to provide steps to verify this on edx staging/production? I think that the net affect should be nothing, but don't know enough about how to verify this to be sure.

[felipemontoya]

@cpennington Since this scenario requires using both microsites and a theme I don't know how could you verify it on staging/production. But maybe you do have such configuration somewhere.

I'll leave a gist of the configuration I added to devstack in case this helps anyone reading this.

[sarina]

@felipemontoya often the more important thing to verify is that this doesn't affect behavior in production.

[nedbat]

@jtauber Another one to fold into your work.

[jtauber]

I'm just writing a test for this, then I'll merge it.

[jtauber]

@felipemontoya can you rebase in light of other changes in this area that I've merged into master?

[felipemontoya]

Sure. I'll get it done sometime today or tomorrow

[jtauber]

@felipemontoya I'll get a test done today for it too.

[felipemontoya]

I rebased it. Locally runs as expected. I didn't find your test, but I ran the relevant tests I found in the courseware djangoapp.

[jtauber]

Sorry for the continued delay. I'm just going to get my footer change landed on master (this afternoon) then test this PR in light of that.

[ovnicraft]

Why casting here ?

[felipemontoya]

Mainly so that you can use it directly in a if clause instead of comparing against {}

[ovnicraft]

You dont need comparing against {}, so python knows {}, [], () as False check:
res = {}
if not res: print "Hello world"

[felipemontoya]

According to this same discussion this was wacky.

[cpennington]

My original suggestion to do this was just that while Python will treat empty data as False, returning an empty dictionary from a function that looks like it should be returning either True or False is strange. By casting inside the function, you make the return value of the function more obviously a boolean.

[natea]

@jtauber Is this ready to merge?

[sarina]

@singingwolfboy @nedbat do either of you know what James was intending to do with this PR? Basically I'm trying to see what was preventing this from being merged, and seeing if I could take over the work.

[singingwolfboy]

@sarina No idea. As far as I'm concerned, this can be merged.

[sarina]

OK - I'm going to just merge then as this seems very small and specific. Thanks DB