Skip to content

Conversation

@rlr
Copy link
Contributor

@rlr rlr commented Feb 10, 2015

I wanted some feedback here before going further.

Instead of hard-coding users and email addresses, I went with two groups. The group of users to track and the group of users to send the report to. Is that cool?

If that's cool, should we create the groups in a migration? I'm pretty sure we've always created new groups in the admin, but I'm game either way.

Does this need a test? It seems like a throwaway script. I'd probably need to refactor it to make it testable. Actually, I can probably poke at the email with django's test infra.

f?

@mythmon
Copy link
Contributor

mythmon commented Feb 10, 2015

I like the idea of using groups for this instead of having a hard coded list. That makes less work for us later.

Since this requires the groups to exist or else it fails, I think we should use a migration to add them (though they can be populated manually).

If it is easy to test, something like "running the cronjob sends an email" that would be cool, but I don't think it is needed, especially if it needs refactoring.

Looks good!

@rlr rlr force-pushed the employee-answers-1085456 branch from e29fe08 to 887eb79 Compare February 11, 2015 16:54
@rlr rlr changed the title [WIP][DO NOT MERGE][bug 1085456] Support Forum metrics email report. [bug 1085456] Support Forum metrics email report. Feb 11, 2015
@rlr
Copy link
Contributor Author

rlr commented Feb 11, 2015

OK, writing a test was worthwhile as I found a typo/bug.

Now this is ready for r?

Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

@rlr
Copy link
Contributor Author

rlr commented Feb 11, 2015

fixord ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realize this is probably going to look really weird in a plain text email, because it will have lots of extra space. Or does send_email deal with that?

@mythmon
Copy link
Contributor

mythmon commented Feb 11, 2015

Besides the white space thing, this looks good. r+ once that is fixed.

This sends a daily email to users in the Support Forum Metrics group.
@rlr rlr force-pushed the employee-answers-1085456 branch from 380cfc2 to d37c030 Compare February 11, 2015 22:08
rlr added a commit that referenced this pull request Feb 11, 2015
[bug 1085456] Support Forum metrics email report.
@rlr rlr merged commit 3be321c into mozilla:master Feb 11, 2015
@rlr rlr deleted the employee-answers-1085456 branch February 11, 2015 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants