The default bug view has changed. See this FAQ.

Account Provisioner: Allow different prices / provider

RESOLVED FIXED in Thunderbird 16.0

Status

Thunderbird
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jb Piacentino, Assigned: bwinton)

Tracking

13 Branch
Thunderbird 16.0

Thunderbird Tracking Flags

(thunderbird14 fixed, thunderbird15 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Attachment #634062 - Flags: ui-review?(mconley)
Attachment #634062 - Flags: review?(mconley)
Attachment #634062 - Flags: feedback?(sancus)
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.
Attachment #634062 - Flags: ui-review?(mconley)
Attachment #634062 - Flags: ui-review+
Attachment #634062 - Flags: review?(mconley)
Attachment #634062 - Flags: review+
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
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.
Attachment #634062 - Attachment is obsolete: true
Attachment #634062 - Flags: feedback?(sancus)
Attachment #634568 - Flags: review?(mconley)
Attachment #634568 - Flags: feedback?(sancus)
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.
Attachment #634568 - Flags: review?(mconley) → review+
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 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.
Attachment #634568 - Flags: feedback?(sancus)
Attachment #634568 - Flags: approval-comm-beta+
Attachment #634568 - Flags: approval-comm-aurora+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-thunderbird14: --- → fixed
status-thunderbird15: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
(Assignee)

Comment 9

5 years ago
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.
Attachment #634667 - Flags: review?(mconley)
Attachment #634667 - Flags: review?(mconley) → review+
You need to log in before you can comment on or make changes to this bug.