Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 974867 - [MMS]Auto suggestion for email address r=Julien #20680

Closed
wants to merge 1 commit into from

Conversation

@try-server-hook

This comment has been minimized.

Copy link

commented Jun 18, 2014

Continuous Integration started. Results

var html;

contact.tel = null;
Settings.supportEmailRecipient = false;

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

nit: please use MockSettings to make it clear we use the mock

also, we don't need to set it because it's false by default, but I don't mind if you keep it to be explicit.

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I corrected it.

var html;

contact.email = null;
Settings.supportEmailRecipient = false;

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

same

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I corrected it.

var html;

contact.tel = null;
Settings.supportEmailRecipient = true;

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

same: please use MockSettings

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I corrected it.

var html;

contact.email = null;
Settings.supportEmailRecipient = true;

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

same

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I corrected it.


contact.tel = null;
contact.email = null;
Settings.supportEmailRecipient = true;

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

same

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I corrected it.

@@ -306,6 +456,72 @@ suite('ContactRenderer', function() {
assert.isTrue(!!li.querySelector(selector));
assert.equal(li.querySelector(selector).lastElementChild, block);
});

test('Rendered no "Tel" and No Support EmailRecipient ', function() {
var html;

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

nit: the var html is not used, please remove

same in the following tests

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I removed it.

require('/test/unit/mock_utils.js');

var MocksHelperForContactsUnitTest = new MocksHelper([
'Settings'

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

and 'Utils' too :)

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I corrected it.

@@ -1,20 +1,30 @@
/*global MockContact, Contacts, fb, MockFbReaderUtilsObj */
/*global MockContact, Contacts, fb, MockFbReaderUtilsObj, MockSettings */

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

you need to add MocksHelper in this line, so that the linter does not report an error

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I corrected it.

require('/test/unit/mock_settings.js');
require('/test/unit/mock_utils.js');

var MocksHelperForContactsUnitTest = new MocksHelper([

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

nit: please start the variable name with a lowercase letter

MockSettings.supportEmailRecipient = true;
var mozContacts = navigator.mozContacts;

Contacts.findContactByString('wontmatch', function(contacts) {

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

wondering why you don't use "z@y.com" here ?

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I corrected it.

MockSettings.supportEmailRecipient = true;
var mozContacts = navigator.mozContacts;

Contacts.findContactByString('O\'Hare', function(contacts) {

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

wondering why you don't use 'a@b.com' here ?

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I corrected it.

@@ -77,6 +79,9 @@ suite('Recipients', function() {
isQuestionable: false,
isInvalid: false
};

fixture.isEmail = MockSettings.supportEmailRecipient &&
Utils.isEmailAddress(this.number);

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

please remove this

instead, add "isEmail: false" to the fixture.

Then add another fixture with an email as "number", "isEmail: true", and add tests about it (so with Settings.supportEmailRecipient = true for these tests)

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I corrected it.


// The fourth and last item is a "cancel" option
assert.equal(items[2].l10nId, 'cancel');
});

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

sorry, I don't understand why we need this test ?

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I removed it.


// The third and last item is a "cancel" option
assert.equal(items[2].l10nId, 'cancel');
});

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

same here, what is special about this test ?

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I removed it.


// The fifth and last item is a "cancel" option
assert.equal(items[4].l10nId, 'cancel');
});

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

same here?

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I removed it.

details = Utils.getContactDetails(number, contacts);
detailsEmail = Utils.getContactDetails(email, contacts);

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

you don't use these 2 new variables, maybe you wanted to use them in the new tests ?

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I removed it.

var calls = MockOptionMenu.calls;

assert.equal(calls.length, 0);
});

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

I think all these new tests do not belong in this patch. I think we'll need to update this when you'll fix the issue when taping the header for an email thread, but it's not fixed yet and thus these tests are useless.

This comment has been minimized.

Copy link
@na-matsumoto

na-matsumoto Jun 19, 2014

Author Contributor

Thank's I removed it.

@@ -2571,11 +2571,13 @@ var ThreadUI = global.ThreadUI = {
var last = this.recipientsList.lastElementChild;
var typed = last && last.textContent.trim();

This comment has been minimized.

Copy link
@julienw

julienw Jun 18, 2014

Contributor

I don't see tests for the new code here.

@na-matsumoto na-matsumoto deleted the KDDI-tech:r3_Bug974867 branch Jun 19, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.