934931 - B2G MMS : use matchPhoneNumbers(...) to match the receiver to update the delivery status.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Gene Lian (I already quit Mozilla)]

Please refer to Bug 914060. We have to use the similar |matchPhoneNumbers(...)| to match the receiver to update the delivery status for the incoming delivery report. Need to improve the logic in |updateMessageDeliveryById(...)|. Otherwise, we would potentially update the delivery status for the wrong receiver.

Comment 1

[Gene Lian (I already quit Mozilla)]

Also, Ctai found out another defect in the updateMessageDeliveryId(...). We should match |messageRecord.deliveryInfo[j].receiver| to update its delivery status instead of relying on the messageRecord.receivers[i]. We may fix that as well in this bug, since it would cause the wrong match for updating the delivery status.

Comment 2

[Gene Lian (I already quit Mozilla)]

Directly assign this one to you if you don't mind. :)

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Sure. I am glad to handle this.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

According to the patch review in Bug 921918, both setMessageReadReportByEnvelopeId() & setMessageDeliveryByEnvelopeId() will apply the same logic for matching Phone number.

mark as duplicated to Bug 921918.

Bevis

Comment 5

[Gene Lian (I already quit Mozilla)]

Oh! I originally thought we can fix that in advance because we don't know how long it will take to finish Bug 921918. Especially, we still have an obvious bug at comment #1.

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

How about if we can't resolved Bug 921918 before tomorrow, then we reopen this bug?

Comment 7

[Gene Lian (I already quit Mozilla)]

Since this one would affect the function of MMS delivery report, which is a important feature, that would be nice if we can fix that before settling down MMS read report.

That sounds good if we can solve that as well at bug 921918 in a short term. Thanks for the update!

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

I think this patch can fix this bug.
https://bugzilla.mozilla.org/attachment.cgi?id=829180

Comment 9

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Hi, Bevis,
Since I can't land bug 921918 right now. Would you mind reopen this bug and take patch https://bug921918.bugzilla.mozilla.org/attachment.cgi?id=829180 for this bug?
That patch should be able fix this bug.
Thanks

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Since the solution will not be included into bug 921918 now, reopen this bug to track.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: 934931_v1.patch, Part 1 to apply matchPhoneNumbers() in updateMessageDeliveryById().

Hi Vicamo,

Because Gene/Ctai are in the business trip, I would like to ask for your help to review this patch.

By ctai's comment, since the patch:part2 in Bug 921918 will not be landed on time due to his business trip, I would like to adopt and refine his patch to fix this bug first:
1. Use unique method matchPhoneNumbers() for the phone number matching.
2. Always compare the receiver info from message.deliveryInfo[] to update the delivery status accordingly.

Regards,
Bevis Tseng

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 830086 [details] [diff] [review]
934931_v1.patch, Part 1 to apply matchPhoneNumbers() in updateMessageDeliveryById().

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1584,5 @@
>      return aMessageRecord.id;
>    },
>  
> +  findReceiversFromDeliveryInfo: function findReceiversFromDeliveryInfo(
> +      receiver, deliveryInfo, modifyFn){

The function is named 'find...', so I'm expecting a function that takes two parameters only.  Something like:

  let entry = findDeliveryInfoEntryByReceiver(deliveryInfo, receiver);
  if (entry) {
    ....
  }

Don't entangle a function with numerous purposes.  The thing we want in a 'find...' function is literally find something out, isn't it?

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #12)
> Comment on attachment 830086 [details] [diff] [review]
> 934931_v1.patch, Part 1 to apply matchPhoneNumbers() in
> updateMessageDeliveryById().
> 
> Review of attachment 830086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
> @@ +1584,5 @@
> >      return aMessageRecord.id;
> >    },
> >  
> > +  findReceiversFromDeliveryInfo: function findReceiversFromDeliveryInfo(
> > +      receiver, deliveryInfo, modifyFn){
> 
> The function is named 'find...', so I'm expecting a function that takes two
> parameters only.  Something like:
> 
>   let entry = findDeliveryInfoEntryByReceiver(deliveryInfo, receiver);
>   if (entry) {
>     ....
>   }
> 
> Don't entangle a function with numerous purposes.  The thing we want in a
> 'find...' function is literally find something out, isn't it?

Thanks for your suggestion.
You are right! I should write these more clearly. :)

Bevis Tseng

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: 934931_v2.patch, Part 1: to apply matchPhoneNumbers() in updateMessageDeliveryById(). r=vyang

wrap the update logic of mms delivery report into the method of |updateDeliveryStatusIntoDeliveryInfo()| for better understanding.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 830661 [details] [diff] [review]
934931_v2.patch, Part 1: to apply matchPhoneNumbers() in updateMessageDeliveryById(). r=vyang

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

“I'm expecting a function that takes two parameters only.“ And I meant it.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch

It seems we still have one deliveryInfo entry in a outgoing MmsMessage for each receiver after bug 928821, which follows we could have duplicated entry in |MmsMessage.deliveryInfo|.  So, I have a forEach func instead and have an additional parameter for callback.

Gene, could you confirm whether that's expected?  Can't find details on http://messaging.sysapps.org/#mmsmessage-interface

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 830729 [details] [diff] [review]
patch

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1584,5 @@
>      return aMessageRecord.id;
>    },
>  
> +  forEachMatchedMmsDeliveryInfo:
> +    function forEachMatchedMmsDeliveryInfo(aDeliveryInfos, aNeedle, aCallback) {

If we have no duplicated entries, we can rename it back to |findMmsDeliveryInfo| and remove the third parameter.

@@ +1696,5 @@
>  
>                isRecordUpdated = true;
>              }
>            } else if (messageRecord.type == "mms") {
> +            let update = function(aEelement) {

typo: aElement.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v2

1) fix typo
2) bail-out early in |function update| and avoid updating record if unnecessary.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v3

Share |updateFunc| between SMS and MMS processes.

Comment 20

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> Created attachment 830729 [details] [diff] [review]
> patch
> 
> It seems we still have one deliveryInfo entry in a outgoing MmsMessage for
> each receiver after bug 928821, which follows we could have duplicated entry
> in |MmsMessage.deliveryInfo|.  So, I have a forEach func instead and have an
> additional parameter for callback.

Bug 928821 just attempts to add deliveryInfo. We should keep the original logic of "each receiver has one deliveryInfo" to ensure the backward-compatibility with Gaia. We can fire another bug to fix that but not for now.

Comment 21

[Gene Lian (I already quit Mozilla)]

Comment on attachment 831561 [details] [diff] [review]
patch : v3

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

This patch won't work actually,

Besides, I hope to use s/receiver/address which is more consistent findParticipantRecordByAddress(...).

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ -1129,5 @@
>          threadRecord.lastMessageSubject = null;
>          cursor.update(threadRecord);
>  
>          cursor.continue();
> -	return;

How could this happen...

@@ +1584,5 @@
>      return aMessageRecord.id;
>    },
>  
> +  forEachMatchedMmsDeliveryInfo:
> +    function forEachMatchedMmsDeliveryInfo(aDeliveryInfos, aNeedle, aCallback) {

info is not countable noun. Let's use s/aDeliveryInfos/aDeliveryInfo to follow the same semantic in other codes. :)

Also, why not s/aNeedle/aAddress/?

Also, I'd prefer s/forEachMatchedMmsDeliveryInfo/updateMmsDeliveryInfoByAddress/.

@@ +1586,5 @@
>  
> +  forEachMatchedMmsDeliveryInfo:
> +    function forEachMatchedMmsDeliveryInfo(aDeliveryInfos, aNeedle, aCallback) {
> +
> +    let typedAddress = MMS.Address.resolveType(aNeedle);

s/aNeedle/aAddress/

@@ +1589,5 @@
> +
> +    let typedAddress = MMS.Address.resolveType(aNeedle);
> +    let normalizedAddress, parsedAddress;
> +    if (typedAddress.type === "PLMN") {
> +      normalizedAddress = PhoneNumberUtils.normalize(receiver, false);

This doesn't work, baby.

s/receiver/aAddress/ or
s/receiver/aAddress/typedAddress.address/?

I'm sure which one is correct.

@@ +1590,5 @@
> +    let typedAddress = MMS.Address.resolveType(aNeedle);
> +    let normalizedAddress, parsedAddress;
> +    if (typedAddress.type === "PLMN") {
> +      normalizedAddress = PhoneNumberUtils.normalize(receiver, false);
> +      parsedAddress = PhoneNumberUtils.parse(normReceiver);

s/normReceiver/normalizedAddress/

@@ +1593,5 @@
> +      normalizedAddress = PhoneNumberUtils.normalize(receiver, false);
> +      parsedAddress = PhoneNumberUtils.parse(normReceiver);
> +    }
> +
> +    for (let element of aDeliveryInfos) {

s/aDeliveryInfos/aDeliveryInfo/

@@ +1594,5 @@
> +      parsedAddress = PhoneNumberUtils.parse(normReceiver);
> +    }
> +
> +    for (let element of aDeliveryInfos) {
> +      let typedInfoReceiver = MMS.Address.resolveType(element.receiver);

s/typedInfoReceiver/typedStoredAddress/

@@ +1595,5 @@
> +    }
> +
> +    for (let element of aDeliveryInfos) {
> +      let typedInfoReceiver = MMS.Address.resolveType(element.receiver);
> +      if (typedAddress.type !== typedInfoReceiver.type) {

s/typedInfoReceiver/typedStoredAddress/

@@ +1600,5 @@
> +        // Not even my type.  Skip.
> +        continue;
> +      }
> +
> +      if (typedAddress.address == typedInfoReceiver.address) {

s/typedInfoReceiver/typedStoredAddress/

@@ +1611,5 @@
> +        // Address type other than "PLMN" must have direct match.  Or, skip.
> +        continue;
> +      }
> +
> +      // Both are of "PLMN" type.

// Both are addresses of "PLMN" type.

@@ +1612,5 @@
> +        continue;
> +      }
> +
> +      // Both are of "PLMN" type.
> +      let normalizedInfoReceiver =

s/normalizedInfoReceiver/normalizedStoredAddress

@@ +1614,5 @@
> +
> +      // Both are of "PLMN" type.
> +      let normalizedInfoReceiver =
> +        PhoneNumberUtils.normalize(element.receiver, false);
> +      let parsedInfoReceiver =

s/parsedInfoReceiver/parsedStoredAddress

@@ +1615,5 @@
> +      // Both are of "PLMN" type.
> +      let normalizedInfoReceiver =
> +        PhoneNumberUtils.normalize(element.receiver, false);
> +      let parsedInfoReceiver =
> +        PhoneNumberUtils.parseWithMCC(normalizedInfoReceiver, null);

s/normalizedInfoReceiver/normalizedStoredAddress

@@ +1617,5 @@
> +        PhoneNumberUtils.normalize(element.receiver, false);
> +      let parsedInfoReceiver =
> +        PhoneNumberUtils.parseWithMCC(normalizedInfoReceiver, null);
> +      if (this.matchPhoneNumbers(normalizedAddress, parsedAddress,
> +                                 normalizedInfoReceiver, parsedInfoReceiver)) {

s/normalizedInfoReceiver/normalizedStoredAddress
s/parsedInfoReceiver/parsedStoredAddress

@@ +1622,5 @@
> +        aCallback(element);
> +      }
> +    }
> +
> +    return null;

Don't need to return null, I think.

@@ +1682,5 @@
>            messageRecord.deliveryIndex = [delivery, messageRecord.timestamp];
>            isRecordUpdated = true;
>          }
>  
>          // Update |messageRecord.deliveryStatus| if needed.

Correct this comment a bit.

// Attempt to update |deliveryStatus| and |deliveryTimestamp| of:
// - the |messageRecord| for SMS.
// - the element(s) in |messageRecord.deliveryInfo| for MMS.

@@ +1684,5 @@
>          }
>  
>          // Update |messageRecord.deliveryStatus| if needed.
>          if (deliveryStatus) {
> +          let updateFunc = function(aTarget) {

Could you please add some words for this callback? Like:

// A callback for updating the deliveyStatus/deliveryTimestamp of each target.

@@ +1708,2 @@
>              } else {
> +              this.forEachMatchedMmsDeliveryInfo(messageRecord.deliveryInfo,

Add one comment:

// If the receiver is specified, we only need to update the element(s) in
// deliveryInfo that match the same receiver.

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: v4

Sorry for all the serious mistakes in previous revision.  Should have a test before review.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/2: test cases

This patches add test cases for SMS only.

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

Attachment: mms test cases

I was to write test cases for MMS as well.  However, it suffers the same problem we have for SMS reference work load in bug 930839 comment 7.  This test script will never be finished.  Just for future reference.

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

Full try: https://tbpl.mozilla.org/?tree=Try&rev=770d03267431

Comment 26

[Gene Lian (I already quit Mozilla)]

Comment on attachment 832763 [details] [diff] [review]
part 1/2: v4

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1584,5 @@
>      return aMessageRecord.id;
>    },
>  
> +  forEachMatchedMmsDeliveryInfo:
> +    function forEachMatchedMmsDeliveryInfo(aDeliveryInfo, aNeedle, aCallback) {

OK. I understood. This function is for iterating through the elements not updating them. My previous suggestion might not be reasonable.

@@ +1612,5 @@
> +        continue;
> +      }
> +
> +      // Both are of "PLMN" type.
> +      let normalizedStoredReceiver =

I feel comfortable to use:

s/normalizedStoredReceiver/normalizedStoredAddress

to have a more consistent semantic within this file.

@@ +1614,5 @@
> +
> +      // Both are of "PLMN" type.
> +      let normalizedStoredReceiver =
> +        PhoneNumberUtils.normalize(element.receiver, false);
> +      let parsedStoredReceiver =

s/parsedStoredReceiver/parsedStoredAddress

@@ +1615,5 @@
> +      // Both are of "PLMN" type.
> +      let normalizedStoredReceiver =
> +        PhoneNumberUtils.normalize(element.receiver, false);
> +      let parsedStoredReceiver =
> +        PhoneNumberUtils.parseWithMCC(normalizedStoredReceiver, null);

s/normalizedStoredReceiver/normalizedStoredAddress

@@ +1617,5 @@
> +        PhoneNumberUtils.normalize(element.receiver, false);
> +      let parsedStoredReceiver =
> +        PhoneNumberUtils.parseWithMCC(normalizedStoredReceiver, null);
> +      if (this.matchPhoneNumbers(normalizedAddress, parsedAddress,
> +                                 normalizedStoredReceiver, parsedStoredReceiver)) {

s/parsedStoredReceiver/parsedStoredAddress
s/normalizedStoredReceiver/normalizedStoredAddress

Comment 27

[Gene Lian (I already quit Mozilla)]

Comment on attachment 832765 [details] [diff] [review]
part 2/2: test cases

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

Nice!

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: v5

Address previous nits.

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a7aa3ff58f78
https://hg.mozilla.org/integration/b2g-inbound/rev/e255bea5f2c1

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a7aa3ff58f78
https://hg.mozilla.org/mozilla-central/rev/e255bea5f2c1

Comment 31

[Andrew Overholt [:overholt]]

This will be part of 1.3 since it landed in gecko 28.