Skip to content

Implement themed enrollment email feature - #6081

Closed
kluo wants to merge 5 commits into
openedx:masterfrom
Stanford-Online:kluo/enrollment-email
Closed

Implement themed enrollment email feature#6081
kluo wants to merge 5 commits into
openedx:masterfrom
Stanford-Online:kluo/enrollment-email

Conversation

@kluo

@kluo kluo commented Nov 27, 2014

Copy link
Copy Markdown
Contributor

Course instructors can choose to send email to users who enroll before or after the course start date. This option is in the Settings -> Schedule & Details page in studio, and is available by setting the ENABLE_ENROLLMENT_EMAIL feature flag.

Allows theming the default enrollment email body by specifying
DEFAULT_PRE_ENROLLMENT_EMAIL and DEFAULT_POST_ENROLLMENT_EMAIL
in environment settings file.

Moves bulk_email models to common in order for studio to access the CourseEmailTemplate model, for sending test email in studio.

*Note @griffresch @jinpa per discussion, this is behind a flag since eventually it would be a global platform feature rather than course-specific.

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @kluo! I've created OSPR-243 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.

@sarina

sarina commented Nov 27, 2014

Copy link
Copy Markdown
Contributor

Something you did with bulk email is causing every test shard to fail...

@openedx-webhooks openedx-webhooks added product review PR requires product review before merging and removed needs triage labels Nov 27, 2014
@sarina

sarina commented Dec 4, 2014

Copy link
Copy Markdown
Contributor

@kluo still working on this PR?

@kluo

kluo commented Dec 4, 2014

Copy link
Copy Markdown
Contributor Author

@sarina Yep, I'm close to fixing the tests, sorry for the delay!

@sarina

sarina commented Dec 4, 2014

Copy link
Copy Markdown
Contributor

@kluo great! Also your PR will need a rebase, FYI.

adampalay and others added 4 commits December 9, 2014 12:59
add retire method to order class
Course instructors can choose to send email to users who enroll
before or after the course start date. This option is in the
Settings -> Schedule & Details page in studio, and is available
by setting the ENABLE_ENROLLMENT_EMAIL feature flag.

Allows theming the default enrollment email body by specifying
DEFAULT_PRE_ENROLLMENT_EMAIL and DEFAULT_POST_ENROLLMENT_EMAIL
in environment settings file.

Moves bulk_email models to common in order for studio to access
the CourseEmailTemplate model, for sending test email in studio.

Initial front-end code by: Ali Reza Sharafat <ali.sharafat@gmail.com>
Co-authored-by: Se Won Jang <swjang@stanford.edu>
@kluo
kluo force-pushed the kluo/enrollment-email branch from 67f7492 to ab84d48 Compare December 9, 2014 22:35
@kluo
kluo force-pushed the kluo/enrollment-email branch from ab84d48 to 21dd99c Compare December 10, 2014 21:57
@kluo

kluo commented Dec 11, 2014

Copy link
Copy Markdown
Contributor Author

@sarina at last, tests have passed!

@sarina sarina removed the product review PR requires product review before merging label Dec 11, 2014
@sarina

sarina commented Dec 11, 2014

Copy link
Copy Markdown
Contributor

@kluo nice! I need to find some of the Solutions team people tomorrow to make sure this code doesn't conflict with any work they're doing - see the OSPR ticket for more on that. Hopefully will get to starting review by end of this week.

@sarina

sarina commented Dec 12, 2014

Copy link
Copy Markdown
Contributor

@chrisndodge - I never found you around this week, and I'm remote next week. Could you take a look at this pull request and let us know if this conflicts with any of the work the white label team is doing? If it is, let's chat about what conflicts and how we might solve the conflicts. Thanks

@chrisndodge

Copy link
Copy Markdown
Contributor

@kluo Thanks for the contribution, looks like an interesting feature. There's a lot to look at here, so I'll try to get through some high level comments over the weekend.

Off the cuff, I see some uses of microsite.get_value() in the CMS implementation. While I appreciate the thoughtfulness behind that, we don't use microsites inside studio, so I might have to make a suggestion to look up microsite configuration by course ORG. I don't have code infront of me right now, so let me get back to you on that shortly.

@sarina

sarina commented Dec 16, 2014

Copy link
Copy Markdown
Contributor

@kluo this feature does NOT seem to be behind a platform-level feature flag, but your description kind of implies it is. Can you clarify?

@sarina

sarina commented Dec 16, 2014

Copy link
Copy Markdown
Contributor

A few broad comments:

  1. Architecturally I don't understand why some files have moved to common/djangoapps/bulk_email while others are moved to lms/djangoapps/bulk_email_lms. I don't like it, either. Is there a way we can keep the bulk email app together in one place? Have you discussed the architecture of this with anyone?
  2. Re the above item: I firmly believe tests should live next to the code they're testing, especially unit tests. We have ways of running common/ tests only in the LMS or the CMS. See for example https://github.com/edx/edx-platform/blob/master/common/djangoapps/student/tests/test_bulk_email_settings.py#L28 - we use this pattern in a lot of places.
  3. You're in a place where you've squashed your commits too much. I'd like to see logically distinct commits, the first of which moves the LMS bulk email app to common, the next implement the actual enrollment email feature.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These docstrings should describe the behavior that would lead to None being returned.

@sarina

sarina commented Dec 16, 2014

Copy link
Copy Markdown
Contributor

@kluo please prepare two brief documents as follows, and post on this PR:

  1. Architecture document. Explain what you've done and why - including all decisions you've made, code you've moved, options you chose not to do, feature flags you've added, views you've added, etc - so that code reviewers can get an overview of your architecture decisions.
  2. Product document. This can be mostly to all screenshots. Walk us through your feature - including ALL screens (Studio settings, any new views in Studio or the LMS, the emails that the users receive). Explain when users receive emails, and how the feature is currently operated (eg, instance-wide feature flags, course wide feature flags, what those flags are named, what value(s) the flags can hold).

Thanks - that will help make reviewing easier. I'm finding it difficult to understand what's going on in this PR.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed community manager review labels Dec 16, 2014
@sarina

sarina commented Jan 5, 2015

Copy link
Copy Markdown
Contributor

@kluo : Hi and happy 2015! 🎊

Are you still planning to work on this pull request? If so, you will really need to prepare the two documents listed above so that I can help run this feature by all the appropriate teams and product managers. It is a better use of your time to prepare these documents before continuing to work on more code - the documents will help the arch council and product teams understand your work and let you more quickly know what you will need to do to get it in a mergeable state.

@kluo

kluo commented Jan 7, 2015

Copy link
Copy Markdown
Contributor Author

@sarina Thanks and you too! Sorry I've been taking care of some other waiting pull requests I had but I'll get to the documents once I'm done with those!

@sarina

sarina commented Feb 17, 2015

Copy link
Copy Markdown
Contributor

@kluo : are you still working on this? If you're not going to pick it up for awhile (it's been 6 weeks), perhaps it's best to close it and reopen at a later time.

@sarina

sarina commented Mar 3, 2015

Copy link
Copy Markdown
Contributor

It's been awhile since this PR has seen an update. I'm going to close it; it can be reopened, or a new PR can be opened, as soon as work resumes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants