Last Comment Bug 763494 - Account Provisioner: Allow different prices / provider
: Account Provisioner: Allow different prices / provider
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: Thunderbird 16.0
Assigned To: Blake Winton (:bwinton) (:☕️)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-11 07:58 PDT by Jb Piacentino
Modified: 2012-06-20 06:17 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
A patch that implements the feature… (1.32 KB, patch)
2012-06-18 09:02 PDT, Blake Winton (:bwinton) (:☕️)
mconley: review+
mconley: ui‑review+
Details | Diff | Splinter Review
Allow different prices per provider. (7.12 KB, patch)
2012-06-19 13:32 PDT, Blake Winton (:bwinton) (:☕️)
mconley: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review
Fix the other nit mconley mentioned. (1.30 KB, patch)
2012-06-19 17:13 PDT, Blake Winton (:bwinton) (:☕️)
mconley: review+
Details | Diff | Splinter Review

Description Jb Piacentino 2012-06-11 07:58:52 PDT
Some providers have different offers, with different prices. 
The suggestion API should allow the display of different prices sections.

A quick hack, in the interim, would consist of duplicating a provider manifest and make it point to a different URL, depending on the targeted offer.
Comment 1 Blake Winton (:bwinton) (:☕️) 2012-06-18 09:02:44 PDT
Created attachment 634062 [details] [diff] [review]
A patch that implements the feature…

Okay, here's the first cut.  For testing, you'll need to change mail.provider.providerList to http://bwinton.latte.ca/Work/provider/list and mail.provider.suggestFromName to http://bwinton.latte.ca/Work/provider/suggestFromName.cgi

Thanks,
Blake.
Comment 2 Mike Conley (:mconley) 2012-06-19 10:44:52 PDT
Comment on attachment 634062 [details] [diff] [review]
A patch that implements the feature…

Review of attachment 634062 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good - just a suggestion below, and a request for documentation.

I'll also take this opportunity to request a Mozmill test for this... can be taken care of in a separate patch if you'd like.

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +761,2 @@
>  
> +        if (address.price && address.price != "0")

I think we can reduce this to:

if (address.price && address.price != "0")
  tmplData.priceStr = stringBundle.get("price", [address.price]);
else if (provider.price && provider.price != "0")
  tmplData.priceStr = stringBundle.get("price", [provider.price]);
else
  tempData.priceStr = stringBundle.get("free");

Also, I think we need a comment block here to explain what's going on.
Comment 3 Blake Winton (:bwinton) (:☕️) 2012-06-19 11:35:51 PDT
(In reply to Mike Conley (:mconley) from comment #2)
> I'll also take this opportunity to request a Mozmill test for this... can be
> taken care of in a separate patch if you'd like.

I'll see if I can whip off some tests in this patch.

> ::: mail/components/newmailaccount/content/accountProvisioner.js
> @@ +761,2 @@
> >  
> > +        if (address.price && address.price != "0")
> 
> I think we can reduce this to:
> 
> if (address.price && address.price != "0")
>   tmplData.priceStr = stringBundle.get("price", [address.price]);
> else if (provider.price && provider.price != "0")
>   tmplData.priceStr = stringBundle.get("price", [provider.price]);
> else
>   tempData.priceStr = stringBundle.get("free");

We can't or providers wouldn't be able to offer some free addresses…  But that leads to the need for documentation, which I will now provide.

> Also, I think we need a comment block here to explain what's going on.

Yeah, I'll add that now.
Comment 4 Blake Winton (:bwinton) (:☕️) 2012-06-19 13:32:53 PDT
Created attachment 634568 [details] [diff] [review]
Allow different prices per provider.

Asking for review for the new tests, mainly.
And re-asking for feedback from sancus.

Changes from the previous patch:
Added comments.
Added a test.
Added a version (=2) to the suggestion api call.
Comment 5 Mike Conley (:mconley) 2012-06-19 14:03:30 PDT
Comment on attachment 634568 [details] [diff] [review]
Allow different prices per provider.

Review of attachment 634568 [details] [diff] [review]:
-----------------------------------------------------------------

Looks solid, except for the erroneous comment block for the subtest.

With that fixed, r=me. Good stuff!

::: mail/test/mozmill/newmailaccount/test-newmailaccount.js
@@ +1260,5 @@
> +  wait_for_modal_dialog("AccountCreation");
> +}
> +
> +/**
> + * Subtest used by test_html_characters_and_ampersands.  This function puts

Nope, this comment block ain't right at all.
Comment 6 Mike Conley (:mconley) 2012-06-19 14:14:15 PDT
Comment on attachment 634568 [details] [diff] [review]
Allow different prices per provider.

Review of attachment 634568 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +754,5 @@
>  
> +        /* Figure out the price to display on the address button, as so:
> +           If there is a per-address price of > 0, use that.
> +           Otherwise, if there is a per-address price of 0, use "Free",
> +           Otherwise, there's no per-address price,

Actually, one last drive-by nit - could you switch these to using the // comment syntax as opposed to the block?  Just for consistencies sake.
Comment 7 Mark Banner (:standard8, limited time in Dec) 2012-06-19 14:52:16 PDT
Comment on attachment 634568 [details] [diff] [review]
Allow different prices per provider.

[Triage Comment]
We wish to take this forward to aurora and beta for further testing and probable roll out in TB 14. So a=me.
Comment 9 Blake Winton (:bwinton) (:☕️) 2012-06-19 17:13:26 PDT
Created attachment 634667 [details] [diff] [review]
Fix the other nit mconley mentioned.

I don't think we especially need to take this on aurora or beta, but having it in trunk seems reasonable.

Thanks,
Blake.

Note You need to log in before you can comment on or make changes to this bug.