588516 - PostCrash Provide Support for emailing after bug fix is shipped

Status: VERIFIED FIXED

(Socorro :: General, task)

Description

[Austin King [:ozten]]

Task #3 from Bug#585593

Comment 1

[Austin King [:ozten]]

Should the email campaign creation form also have a Product drop down?

[Firefox
 Thunderbird
 Camino
 SeaMonkey]

The emails would be limited by the crash signature, product, start date, and end date.

This is kind of a gecko versus product question. Also, should this be limited to version? (I don't think so, but thought I should ask).

Comment 2

[David Tenser [:djst]]

For the first version, we can just focus on Firefox since that's what we'll use this for. Don't see a problem with having a dropdown for product, though, as long as that doesn't jeopardize our schedule to push this out in Q3.

Not sure what the question about version limitation means? The way I understand it, if all bugs associated with a specific signature is fixed, that's our trigger to send out emails to users. At that point we would craft an email saying e.g.:

"You submitted a report on [date] and we'd like to inform you that as of Firefox version x.y.z, this problem is now fixed."

Not sure if this answers your questions though -- Laura might be better at this. :)

Comment 3

[Austin King [:ozten]]

(In reply to comment #2)

Thanks David!

Sounds good, I will add a product drop down. It won't add much time to the work.

I will not add a version field as bug fixes effect many previous versions.

Comment 4

[Austin King [:ozten]]

Responsys has a SOAP API, not SMTP. It typically used like MailChimp with list management, users logging into it, etc.

SocketLabs provides SMTP, but has no opt-out database/unsubscribe urls. it is more of a transactional email system. We will be using it for this feature.

We will go with SocketLabs and build the missing pieces in Bug#598148.

Comment 5

[chris hofmann]

for the "we fixed your bug" case how will we handle the situation where a user might have crashed with two or three (or more for a major release) different signature?   will we send two, three, or more "we fixed your bug" e-mails?

that beings to look like spam to me.  I'm still not convinced of the value of these e-mails that are unconnected to crash event in time, or context.

Comment 6

[Austin King [:ozten]]

Attachment: First stab at adding PostCrashEmail over 1.8 codebase

Attached is a single patch for Python and PHP code.

Caveats:
* Patch is against my branch which is 1.8 based
* No new unit tests (will work on them during code review)

I'll create a version of this patch that applies cleanly against the 1.7reversion branch.

Comment 7

[K Lars Lohn [:lars] [:klohn]]

Comment on attachment 479429 [details] [diff] [review]
First stab at adding PostCrashEmail over 1.8 codebase

major - socorro/services/emailCampaignCreate.py:51 has a problem in the use of the module socorro.lib.ConfigurationManager.  That module produces a context object that needs to operate as a singleton.  While in our current production environment, it will not likely cause trouble, it will cause difficulties when running the middleware without wsgi/Apache.  When running standalone, it is common (in my testing environment) to modify the configuration through command line arguments.  The configuration manager will take commandline args and validate them using the supplied config module as a guide.  By introducing a second configuration manager with a different config module, there is a contradiction.  My supplied commandline argument may be valid for the original configuration manager, it won't be for the second one.  That second one will then raise "NotAnOption" exceptions.  There is no facility in the configuration manager to tell it to not look at the commandline arguments.

The way around this problem is to have the smtpconfig.py module imported by the existing webapiconfig.py module.  The smtpconfig parameters will get merged into the rest of the app's configuration.  It then is available to the EmailCampaignCreate service via the self.context variable without having to have a second configuration manager.  You may have to change the names of some parameters to avoid name collision.

troublesome - socorro/services/emailCampaigns.py:67 - try: finally: missing around the database connection.  A database exception will make it skip the closing of the connection.

worry wort - I'm concerned about transactional semantics when using postgres connections via pgbouncer.  These are the first middleware services that actually write to the database.  I see some commits in the code, but there are no code paths with rollbacks.  Under pgbouncer, what is the transactional behavior when a connection to postgres is released?  Do they automatically rollback or commit?  We need to make sure that the behavior is the same both with and without pgbouncer.

minor - socorro/services/emailCampaigns.py:60 - campaign is created as a DotDict, in subsequent lines, it is used like a regular dict using [] notation instead of . notation.  Functionally it doesn't matter, but why bother with DotDict if its only feature over dict isn't going to be used?

Comment 8

[Austin King [:ozten]]

Attachment: Second stab on 1.7reversion codebase

Patch based on 1.7reversion branch.

@lars - thanks for the review.

Removed 2nd Config Manager

Added try, finally to emailCampaigns

pgbouncer is configured with pool_mode=session. This is the least aggressive mode for pooling and should not affect transactional semantics.

Updated DotDict usage.

Comment 9

[K Lars Lohn [:lars] [:klohn]]

ozten, this latest patch does not appear to be complete.  The python files for the new middleware services are not present.

Comment 10

[Austin King [:ozten]]

(In reply to comment #9)
Sigh. I didn't do svn add in this new repo...

I'll get a fresh patch together.

Comment 11

[Austin King [:ozten]]

Attachment: Third attempt with missing files

Comment 12

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Comment on attachment 479873 [details] [diff] [review]
Third attempt with missing files

From a code perspective, this looks really good.  Great work Austin.

I'm a tad concerned about the code merge.  I know that this code is in the 1.7.4 branch, but I recognized code that was written and committed in the 1.8 branch in both the Admin Controller and the Web Service library.  I think it will be okay, but we'll need to test product/version editing in the admin panel to be sure.

From a security perspective ( referencing #600385 ), I would agree that we need one extra step of protection to prevent brute force opt-outs.  Adding a captcha here would help mitigate this issue.  Regarding, the other 2 recommendations, it looks like you're already using unpredictable IDs, and I think creating user accounts for this would be a bit overkill.

r- this...  Once you've verified the product/version editing and added a captcha, r+

Comment 13

[Austin King [:ozten]]

(In reply to comment #12)

@ryansnyder - thanks for the code review.

> I recognized code that was written and committed in the 1.8

Which functions or lines of code are 1.8? I will revert them.

> but we'll need to test product/version editing in the admin panel to be sure.

I'd rather revert the code (and of course test) than to rely on testing to discover this merge issue.

Comment 14

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

It will probably be easiest to do an svn diff between the previous commits and your current work.  Here are the commits I made:

Admin Controller:
https://bugzilla.mozilla.org/show_bug.cgi?id=575707#c8
(Committed revision 2232)

Web Service Library:
https://bugzilla.mozilla.org/show_bug.cgi?id=584136#c14
(Committed revision 2360)

Comment 15

[Austin King [:ozten]]

Attachment: Fourth attempt with ReCAPTCHA

Leaving Attachment#479873 [details] [diff] since it's r+ for python and database...

New patch:
admin controller - good catch, code reverted
Web_Service library - read my lips, no new taxes err code

Added ReCAPTCHA module and enhanced email/subscribe and email/update_status to require valid captcha answer.

Updated SocorroUpgrade with instructions for copying new config/recaptcha.php

Comment 16

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Comment on attachment 480427 [details] [diff] [review]
Fourth attempt with ReCAPTCHA

Looks good.  Thanks Austin.

Also to note - the Recaptcha module is a Kohana module that is being reused from the mozillaservice.org project.

Comment 17

[Robert Helmer [:rhelmer]]

Comment on attachment 480427 [details] [diff] [review]
Fourth attempt with ReCAPTCHA

FWIW the schema.py changes introduce a new failure in socorro/unittest/database/testSchema.py 
There is one existing failure in there already, maybe this test should be disabled if it's not useful...

>Index: webapp-php/application/controllers/admin.php
>===================================================================
>--- webapp-php/application/controllers/admin.php	(revision 2548)
>+++ webapp-php/application/controllers/admin.php	(working copy)
(...)
>+        $count = preg_match_all('/\*\|[a-zA-Z_ ]*\|\*/', $body, $variables);
>+
>+        if (0 < $count) {
>+            for ($i = 0; $i < $count; $i++) {
>+                $var = $variables[0][$i];
>+                if (in_array($var, $valid_variables)) {
>+                    if ($var == $valid_variables[0]) {
>+                        $unsubscribe_seen = true;
>+                    }
>+                } else {
>+                    $validation->add_error($field, 'valid_unknown_variable');
>+                }
>+            }
>+        }

Is that "if" really doing anything here? 
"for ($i = 0; $i < $count; ...)" should be fine if $count is 0 initially.

Everything else LGTM.

Comment 18

[Austin King [:ozten]]

(In reply to comment #17)
yes, this unit test had already been removed in the 1.8 branch.

Comment 19

[Austin King [:ozten]]

Committing to r2561.
Adding         scripts/config/smtpconfig.py.dist
Sending        scripts/config/webapiconfig.py.dist
Sending        scripts/webservices.py
Sending        socorro/database/schema.py
Adding         socorro/services/emailCampaign.py
Adding         socorro/services/emailCampaignCreate.py
Adding         socorro/services/emailCampaignVolume.py
Adding         socorro/services/emailCampaigns.py
Adding         socorro/services/emailSubscription.py
Sending        socorro/webapi/webapiService.py
Sending        webapp-php/application/config/codebases.php
Sending        webapp-php/application/config/config.php-dist
Sending        webapp-php/application/config/routes.php
Sending        webapp-php/application/config/search.php
Sending        webapp-php/application/controllers/admin.php
Adding         webapp-php/application/controllers/email.php
Adding         webapp-php/application/i18n
Adding         webapp-php/application/i18n/en_US
Adding         webapp-php/application/i18n/en_US/email_form_errors.php
Sending        webapp-php/application/libraries/Web_Service.php
Adding         webapp-php/application/views/admin/confirm_email.php
Adding         webapp-php/application/views/admin/email.php
Adding         webapp-php/application/views/admin/email_campaign.php
Sending        webapp-php/application/views/admin/index.php
Adding         webapp-php/application/views/common/form_errors.php
Adding         webapp-php/application/views/common/recent_email_campaigns.php
Adding         webapp-php/application/views/email
Adding         webapp-php/application/views/email/subscription.php
Sending        webapp-php/css/datePicker.css
Sending        webapp-php/css/screen.css
Sending        webapp-php/js/socorro/admin.js
Adding         webapp-php/modules/recaptcha
Adding         webapp-php/modules/recaptcha/config
Adding         webapp-php/modules/recaptcha/config/recaptcha.php-dist
Adding         webapp-php/modules/recaptcha/docs
Adding         webapp-php/modules/recaptcha/docs/recaptchalib-LICENSE
Adding         webapp-php/modules/recaptcha/docs/recaptchalib-README
Adding         webapp-php/modules/recaptcha/helpers
Adding         webapp-php/modules/recaptcha/helpers/recaptcha.php
Adding         webapp-php/modules/recaptcha/hooks
Adding         webapp-php/modules/recaptcha/hooks/init.php
Adding         webapp-php/modules/recaptcha/vendor
Adding         webapp-php/modules/recaptcha/vendor/recaptchalib.php
Sending        webapp-php/tests/CorrelationTest.php
Sending        webapp-php/tests/CrashReportModelTest.php
Sending        webapp-php/tests/SearchReportHelperTest.php

Comment 20

[Austin King [:ozten]]

Merged and commited on trunk r2565.

Comment 21

[Austin King [:ozten]]

Test Data:
Please see https://docs.google.com/document/pub?id=1_Rhg1n3ac-rvMkjow_5nCJJv6up5oqineue7cmHcBpE

We can create more test data as needed. Only test /admin/email with dates between Jan 2th 2010 through Jan 30th 2010!

IT will switch to SocketLabs (Bug#603806) sometime this week, but can start testing with the local SMTP server.

Comment 22

[Vishal Kamdar [:vish_moz]]

Verified, FIXED.
Stephen and I tested this, https://wiki.mozilla.org/QA/Execution/Web_Testing/Socorro/Test_Plan/1.7.5