763494 - Account Provisioner: Allow different prices / provider

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jb Piacentino]

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) (:☕️)]

Attachment: 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) (:⚙️)]

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) (:☕️)]

(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) (:☕️)]

Attachment: 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) (:⚙️)]

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) (:⚙️)]

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)]

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 8

[Mark Banner (:standard8)]

Checked in:

https://hg.mozilla.org/comm-central/rev/52c24582e9d6
https://hg.mozilla.org/releases/comm-aurora/rev/f83715de5139
https://hg.mozilla.org/releases/comm-beta/rev/cd7e03be21f6

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: 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.