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

Implement themed enrollment email feature #6081

Closed
wants to merge 5 commits into from

Conversation

kluo
Copy link
Contributor

@kluo kluo commented Nov 27, 2014

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

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
Copy link
Contributor

sarina commented Nov 27, 2014

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
Copy link
Contributor

sarina commented Dec 4, 2014

@kluo still working on this PR?

@kluo
Copy link
Contributor Author

kluo commented Dec 4, 2014

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

@sarina
Copy link
Contributor

sarina commented Dec 4, 2014

@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 <[email protected]>
Co-authored-by: Se Won Jang <[email protected]>
@kluo
Copy link
Contributor Author

kluo commented Dec 11, 2014

@sarina at last, tests have passed!

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

sarina commented Dec 11, 2014

@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
Copy link
Contributor

sarina commented Dec 12, 2014

@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
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
Copy link
Contributor

sarina commented Dec 16, 2014

@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
Copy link
Contributor

sarina commented Dec 16, 2014

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.

@@ -146,6 +146,30 @@ def get_lms_link_for_about_page(course_key):
)


def get_lms_link_for_dashboard():
"""Returns the url to the lms dashboard."""
Copy link
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
Copy link
Contributor

sarina commented Dec 16, 2014

@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
Copy link
Contributor

sarina commented Jan 5, 2015

@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
Copy link
Contributor Author

kluo commented Jan 7, 2015

@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
Copy link
Contributor

sarina commented Feb 17, 2015

@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
Copy link
Contributor

sarina commented Mar 3, 2015

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