Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Including #10623 into WL-245 #11224

Merged
merged 4 commits into from
Jan 15, 2016

Conversation

felipemontoya
Copy link
Member

This PR updates the code in the microsite_configuration djangoapp to match the recent changes in PR #10623

@openedx-webhooks
Copy link

Thanks for the pull request, @felipemontoya! I've created OSPR-1073 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Jan 14, 2016
@felipemontoya
Copy link
Member Author

@mattdrayer and @ziafazal These are the changes I mentioned in #11073. And this PR is a created against the intermediate branch. Feel free to merge it or cherry pick the commits

@mattdrayer
Copy link
Contributor

@felipemontoya if we merge this into #11073 should we close #10623?

@felipemontoya
Copy link
Member Author

@mattdrayer, no #10623 is already merged. If you merge this, then #11073 will not break the functionality of #10623

@mattdrayer
Copy link
Contributor

ah, of course, sorry, juggling too many PRs :)

On Wed, Jan 13, 2016 at 9:28 PM, Felipe Montoya notifications@github.com
wrote:

@mattdrayer https://github.com/mattdrayer, no #10623
https://github.com/edx/edx-platform/pull/10623 is already merged. If
you merge this, then #11073
https://github.com/edx/edx-platform/pull/11073 will not break the
functionality of #10623 https://github.com/edx/edx-platform/pull/10623


Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/11224#issuecomment-171511428.

Matthew Drayer
Solutions Architect
edX | MIT
mattdrayer@edx.org

@@ -41,6 +41,8 @@ def run():

add_mimetypes()

microsite.enable_microsites(log)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felipemontoya I'm not able to understand the usage of this statement. Why we need to do the same thing once before django.setup() and once afterwards? Could you please explain a little?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. It goes like this, we are doing similar things, but for different components.

We currently use 2 template rendering engines, mako and django_templates, and one of them (django templates), requires the directories be added before the django.setup(). Mako requires the directories to be added after the django setup. That is why we have this that modified the DIRS for the django_templates setup before the django.setup() and we also have this which does the same for mako after the django.setup(). I did left this line even when it was not necessary, just to be sure, but we can remove it and nothing should be affected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziafazal I tried it locally and also removed the duplicated line

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felipemontoya it would be nice if you can post these comments inside code.

@mattdrayer
Copy link
Contributor

jenkins run python

@mattdrayer
Copy link
Contributor

jenkins run lettuce

@mattdrayer
Copy link
Contributor

@felipemontoya -- running into some test failures on this branch -- can you take a look when you have a moment?

@mattdrayer
Copy link
Contributor

darn, (unrelated) test timeout

jenkins run bokchoy

@mattdrayer
Copy link
Contributor

@felipemontoya this changeset now LGTM -- 👍! We can squash the commits and merge once @ziafazal approves.

@felipemontoya
Copy link
Member Author

I guess you will also want to squash the branch used for PR #11073. So if @ziafazal approves then you can merge it and squash the ziafazal/WL-245 branch altogether

@ziafazal
Copy link
Contributor

LGTM 👍

@ziafazal ziafazal merged this pull request into openedx:ziafazal/WL-245 Jan 15, 2016
@felipemontoya felipemontoya deleted the fmo/ziafazal/WL-245 branch October 22, 2019 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants