Closed Bug 885679 Opened 7 years ago Closed 6 years ago

B2G MMS: Add 'subject' to {Thread} object.

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
1.3 Sprint 4 - 11/8
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: borjasalguero, Assigned: vicamo)

References

Details

(Keywords: dev-doc-needed, Whiteboard: MMS_TEF, [u=commsapps-user c=messaging p=0] [FT:RIL])

Attachments

(3 files, 6 obsolete files)

For showing in the thread-list of MMS App the subject we need to have this info available in 'Thread' object when requesting 'getThreads'.
Blocks: 885680
blocking-b2g: --- → leo?
Whiteboard: MMS_TEF
Whiteboard: MMS_TEF → MMS_TEF, TaipeiWW
Summary: [B2G] Add 'subject' to {Thread} object. → B2G MMS: Add 'subject' to {Thread} object.
Please clearly define what should thread::subject be in following cases:

1. thread messages are all SMS,
2. some are SMS and some are MMS,
3. thread messages are all MMS but with different subjects
Over to dietrich to make the call on blocking, given this will require late functionality/UX. Sounds like a blocker, but we want to make sure we _need_ it.
Flags: needinfo?(dietrich)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #1)
> Please clearly define what should thread::subject be in following cases:
> 
> 1. thread messages are all SMS,
> 2. some are SMS and some are MMS,
> 3. thread messages are all MMS but with different subjects

I think Borja is talking about the subject of the last message in the thread. Right?
Assignee: nobody → gene.lian
Blocks: b2g-mms
Flags: needinfo?(fbsc)
Yep. But I think that this is not a blocker right now, but we should handle the subject in the future (currently the composer has no subject for example, that's why I think it's not a blocker). Dietrich, could you confirm if this is our roadmap for v1.2? Thanks!
Flags: needinfo?(fbsc)
blocking-b2g: leo? → koi?
Whiteboard: MMS_TEF, TaipeiWW → MMS_TEF
Assignee: gene.lian → vyang
Attached patch part 1/2: interface changes (obsolete) — Splinter Review
Attached patch part 2/2: dom & backend changes (obsolete) — Splinter Review
Still need detailed information about what should a thread subject be under various conditions as described in my comment 1.  Will only request for review after that being addressed.
Whiteboard: MMS_TEF → MMS_TEF, [u=commsapps-user c=messaging p=0]
Vicamo, who do you need that info from? UX?
Flags: needinfo?(dietrich)
(In reply to Dietrich Ayala (:dietrich) from comment #7)
> Vicamo, who do you need that info from? UX?

Let's ask the reporter.
Flags: needinfo?(fbsc)
In this case we need input from Product & UX Team. Subject is handled in several ways depending on the platform, and we need to agree how to make this happen in our Firefox OS :)
Flags: needinfo?(fbsc)
Let me try.

The subject should reflect the last message in the thread, just like the timestamp. Therefore:

1. last message is a SMS
-> thread.subject is null

2. last message is a MMS without a subject
-> thread.subject is null

3. last message is a MMS with a subject
-> thread.subject = lastMessage.subject


Generally, this is it: thread.subject = lastMessage.subject.

Ayman, can you confirm this (so that Gecko dev can move forward) ?
Flags: needinfo?(aymanmaat)
No longer blocks: 885680
(In reply to Julien Wajsberg [:julienw] from comment #10)
> Let me try.
> 
> The subject should reflect the last message in the thread, just like the
> timestamp. Therefore:
> 
> 1. last message is a SMS
> -> thread.subject is null
> 
> 2. last message is a MMS without a subject
> -> thread.subject is null
> 
> 3. last message is a MMS with a subject
> -> thread.subject = lastMessage.subject
> 
> 
> Generally, this is it: thread.subject = lastMessage.subject.
> 
> Ayman, can you confirm this (so that Gecko dev can move forward) ?

I agree, this seems a pragmatic approach to me.
Flags: needinfo?(aymanmaat)
Comment on attachment 770655 [details] [diff] [review]
part 1/2: interface changes

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

::: dom/mobilemessage/interfaces/nsIDOMMozMobileMessageThread.idl
@@ +10,5 @@
>    // Unique identity of the thread.
>    readonly attribute unsigned long long id;
>  
> +  // Message subject.
> +  readonly attribute DOMString          subject;

I'd prefer s/subject/lastMessageSubject, just like lastMessageType.
blocking-b2g: koi? → 1.3?
Attached patch patch 1/3: interfaces (obsolete) — Splinter Review
s/subject/lastMessageSubject/
Attachment #770655 - Attachment is obsolete: true
Attachment #812983 - Flags: superreview?(gene.lian)
Test cases to come.
Attachment #770656 - Attachment is obsolete: true
Attachment #812984 - Flags: feedback?(gene.lian)
Attachment #812983 - Flags: superreview?(gene.lian) → superreview+
Comment on attachment 812984 [details] [diff] [review]
patch 2/3: dom & backend changes : WIP

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

Simple enough to directly give review+. :) But I think there might be some tests to fix.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +814,5 @@
> +      // actually means 'threadRecord.body'.  Swap the two values first.
> +      threadRecord.body = threadRecord.subject;
> +      delete threadRecord.subject;
> +
> +      if (threadRecord.lastMessageType != "mms") {

Add one simple comment before this line:

// Only MMS supports subject so assign null for non-MMS one.

or something you like.

@@ +825,5 @@
> +
> +      messageStore.get(threadRecord.lastMessageId).onsuccess = function(event) {
> +        let messageRecord = event.target.result;
> +        let subject = messageRecord.headers.subject;
> +        threadRecord.lastMessageSubject = subject || null;

This will convert empty string to null but should be fine... I assume you're hoping to save null as default for the thread DB.

@@ +1184,4 @@
>            let needsUpdate = false;
>  
>            if (threadRecord.lastTimestamp <= timestamp) {
> +            let messageSubject;

We might not have to use an extra variable by writing it to:

threadRecord.lastMessageSubject = (aMessageRecord.type == "mms")
                                ? aMessageRecord.headers.subject : null;

-----
After some thoughts... Maybe you're intentionally doing this to deal with empty string. OK! Your codes are good to go.
Attachment #812984 - Flags: feedback?(gene.lian) → review+
blocking a committed 1.3 user story. 1.3+
blocking-b2g: 1.3? → 1.3+
Target Milestone: --- → 1.3 Sprint 4 - 11/8
This is committed feature. Need to move fast. Can we finish this in a couple of days? Since we only lack test cases.
Flags: needinfo?(vyang)
Re-gen from HG.
Attachment #812983 - Attachment is obsolete: true
Attachment #824614 - Flags: superreview+
Flags: needinfo?(vyang)
Re-gen from HG and address comment 15.
Attachment #812984 - Attachment is obsolete: true
Attachment #824616 - Flags: review+
Attached patch part 3/3: test cases (obsolete) — Splinter Review
Test cases for thread subject.  Full try: https://tbpl.mozilla.org/?tree=Try&rev=f91bbb61b069
Attachment #824617 - Flags: review?(gene.lian)
Comment on attachment 824617 [details] [diff] [review]
part 3/3: test cases

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

::: dom/mobilemessage/tests/marionette/head.js
@@ +27,5 @@
> +
> +  return deferred.promise;
> +}
> +
> +function sendSingleSmsWithSuccess(aReceiver, aText) {

How about s/sendSingleSmsWithSuccess/sendSmsWithSuccess/?

It's not very clear to me what does "Single" mean.

@@ +44,5 @@
> +
> +function sendMmsWithFailure(aMmsParameters) {
> +  let deferred = Promise.defer();
> +
> +  manager.onfailed = function (event) {

Looks awesome!

The only thing I don't understand is |manager.onfailed|. Where does the "onfailed" come from?

::: dom/mobilemessage/tests/marionette/test_thread_subject.js
@@ +6,5 @@
> +
> +const PHONE_NUMBER = "+1234567890";
> +
> +// Have a long long subject causes the send fails, so we don't need
> +// networking here.

// What we want to check is the subject of the last message in a thread.
// We don't really need to send out a real MMS. To do so, we intentionally
// send an MMS with a long long subject, which will cause the send fails,
// so we don't need real networking here.

@@ +8,5 @@
> +
> +// Have a long long subject causes the send fails, so we don't need
> +// networking here.
> +const MMS_MAX_LENGTH_SUBJECT = 40;
> +const SUBJECT =

Please add one blank line above this var.

@@ +10,5 @@
> +// networking here.
> +const MMS_MAX_LENGTH_SUBJECT = 40;
> +const SUBJECT =
> +  "Hello" + (new Array(MMS_MAX_LENGTH_SUBJECT).join(" ")) + "World!";
> +const MMS_PARAMETER = {

Please add one blank line above this var.

@@ +21,5 @@
> +  let deferred = Promise.defer();
> +
> +  log("Testing thread subject: " + aProgressStr);
> +
> +  sendSingleSmsWithSuccess(PHONE_NUMBER, "text")

s/sendSingleSmsWithSuccess/sendSmsWithSuccess/?

@@ +23,5 @@
> +  log("Testing thread subject: " + aProgressStr);
> +
> +  sendSingleSmsWithSuccess(PHONE_NUMBER, "text")
> +    .then(function(message) {
> +      log("  SMS sent, retrieving thread of id " + message.threadId);

s/thread of id/thread by id/

@@ +44,5 @@
> +
> +  // We use a long long message subject so it will always fail.
> +  sendMmsWithFailure(MMS_PARAMETER)
> +    .then(function(message) {
> +      log("  MMS sent, retrieving thread of id " + message.threadId);

Ditto.
Attachment #824617 - Flags: review?(gene.lian) → review+
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #22)
> part 3/3: test cases
> Review of attachment 824617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/tests/marionette/head.js
> @@ +27,5 @@
> > +function sendSingleSmsWithSuccess(aReceiver, aText) {
> 
> How about s/sendSingleSmsWithSuccess/sendSmsWithSuccess/?
> It's not very clear to me what does "Single" mean.

I was to have a sendSingleSmsWithSuccess, a sendMultipleSmsWithSuccess, and a sendSmsWithSuccess that calls one of previous two depending on given parameter.  But, yes, they're removed for now because they're not referenced yet.

> @@ +44,5 @@
> > +
> > +function sendMmsWithFailure(aMmsParameters) {
> > +  let deferred = Promise.defer();
> > +
> > +  manager.onfailed = function (event) {
> 
> Looks awesome!
> 
> The only thing I don't understand is |manager.onfailed|. Where does the
> "onfailed" come from?

http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl#47
In DOMRequest.onerror events, we have only a DOMError and no the sent message object.  In order to keep the symmetry between send{Mms,Sms}WithSuccess and send{Mms,Sms}WithFailure, I listen to the onfailed event of navigator.mozMobileMessage to get that sent message object as well.
Oh! We forgot we have onfailed event handler. Cool!
Attached patch part 3/3: test cases : v2 (obsolete) — Splinter Review
Sorry, this revision changes a lot so I think another review is necessary.

1) Address previous comment, s/sendSingleSmsWithSuccess/sendSmsWithSuccess/
2) Add comments for each convenient function in head.js for future reference.
3) Ensure it really works with MOZ_B2G_RIL disabled, that is, |manager| could be undefined.  We also need a reject handler at the end.
4) Refactor getThreadById, deleteMessagesById, deleteMessages, and deleteAllMessages.  Only create new Promise whenever necessary.
5) Remove sendSmsToEmulator.  Yes, we will certainly need it when we convert other SMS/MMS test scripts to share some code, but it's not today.
Attachment #824617 - Attachment is obsolete: true
Attachment #826289 - Flags: review?(gene.lian)
Whiteboard: MMS_TEF, [u=commsapps-user c=messaging p=0] → MMS_TEF, [u=commsapps-user c=messaging p=0] [FT:RIL]
Comment on attachment 826289 [details] [diff] [review]
part 3/3: test cases : v2

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

Nice patch!!!

::: dom/mobilemessage/tests/marionette/head.js
@@ +75,5 @@
> +/* Send a MMS message with specified parameters.  Resolve if it fails, reject
> + * otherwise.
> + *
> + * Forfill params:
> + *   error -- a DOMError.

Isn't that an MmsMessage?

@@ +86,5 @@
> + */
> +function sendMmsWithFailure(aMmsParameters) {
> +  let deferred = Promise.defer();
> +
> +  manager.onfailed = function (event) {

nit: drop the blank after "function".

::: dom/mobilemessage/tests/marionette/test_thread_subject.js
@@ +52,5 @@
> +    .then(testSms.bind(null, "SMS..MMS..SMS"))
> +    .then(deleteAllMessages)
> +    .then(testMms.bind(null, "MMS"))
> +    .then(testSms.bind(null, "MMS..SMS"))
> +    .then(testMms.bind(null, "MMS..SMS..MMS"));

Could you please add one more case for sending consecutive MMS (maybe, 2)? So that we can make sure the last MMS subject can be updated without the interference of SMS. Thank you!
Attachment #826289 - Flags: review?(gene.lian) → review+
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #26)
> part 3/3: test cases : v2
> Review of attachment 826289 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/mobilemessage/tests/marionette/head.js
> @@ +75,5 @@
> > +/* Send a MMS message with specified parameters.  Resolve if it fails, reject
> > + * otherwise.
> > + *
> > + * Forfill params:
> > + *   error -- a DOMError.
> 
> Isn't that an MmsMessage?

You're right.  The comment has to be updated.  Thanks!
address review comment 26:
1. update documentation for |sendMmsWithFailure()|
2. s/function (/function(/
3. add test cases for three continuous SMS messages with different text strings and for three continuous MMS messages with different subjects.
Attachment #826289 - Attachment is obsolete: true
Attachment #827975 - Flags: review+
Comment on attachment 824616 [details] [diff] [review]
patch 2/3: dom & backend changes : v2

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +811,5 @@
> +
> +      let threadRecord = cursor.value;
> +      // We have defined 'threadRecord.subject' in upgradeSchema7(), but it
> +      // actually means 'threadRecord.body'.  Swap the two values first.
> +      threadRecord.body = threadRecord.subject;

I don't get this: didn't have a body previously already? I think this overwrites any body that was here before.

As a matter of fact, we use a "body" in the SMS app and old databases get "undefined" now with the current code.

I'm filing a bug about this.
Depends on: 945711
You need to log in before you can comment on or make changes to this bug.