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

RESOLVED FIXED in 1.3 Sprint 5 - 11/22

Status

Firefox OS
RIL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gene Lian (I already quit Mozilla), Assigned: vicamo)

Tracking

unspecified
1.3 Sprint 5 - 11/22
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

Comment 2

5 years ago
Directly assign this one to you if you don't mind. :)
Assignee: nobody → btseng
Sure. I am glad to handle this.
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 921918
(Reporter)

Comment 5

5 years ago
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.
How about if we can't resolved Bug 921918 before tomorrow, then we reopen this bug?
(Reporter)

Comment 7

5 years ago
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!
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
Flags: needinfo?(btseng)
Since the solution will not be included into bug 921918 now, reopen this bug to track.
Status: RESOLVED → REOPENED
Flags: needinfo?(btseng)
Resolution: DUPLICATE → ---
Created attachment 830086 [details] [diff] [review]
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
Attachment #830086 - Flags: review?(vyang)
Target Milestone: --- → 1.3 Sprint 5 - 11/22
(Assignee)

Comment 12

5 years ago
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?
Attachment #830086 - Flags: review?(vyang)
(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
Created attachment 830661 [details] [diff] [review]
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.
Attachment #830086 - Attachment is obsolete: true
Attachment #830661 - Flags: review?(vyang)
(Assignee)

Comment 15

5 years ago
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.
Attachment #830661 - Flags: review?(vyang) → review-
(Assignee)

Updated

5 years ago
Assignee: btseng → vyang
(Reporter)

Updated

5 years ago
Blocks: 894271
(Assignee)

Comment 16

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

Gene, could you confirm whether that's expected?  Can't find details on http://messaging.sysapps.org/#mmsmessage-interface
Attachment #830661 - Attachment is obsolete: true
Attachment #830729 - Flags: feedback?(gene.lian)
(Assignee)

Comment 17

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

Updated

5 years ago
Blocks: 921918
(Assignee)

Comment 18

5 years ago
Created attachment 831254 [details] [diff] [review]
patch : v2

1) fix typo
2) bail-out early in |function update| and avoid updating record if unnecessary.
Attachment #830729 - Attachment is obsolete: true
Attachment #830729 - Flags: feedback?(gene.lian)
Attachment #831254 - Flags: review?(gene.lian)
(Assignee)

Comment 19

5 years ago
Created attachment 831561 [details] [diff] [review]
patch : v3

Share |updateFunc| between SMS and MMS processes.
Attachment #831254 - Attachment is obsolete: true
Attachment #831254 - Flags: review?(gene.lian)
Attachment #831561 - Flags: review?(gene.lian)
(Reporter)

Comment 20

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

Comment 21

5 years ago
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.
Attachment #831561 - Flags: review?(gene.lian) → review-
(Assignee)

Comment 22

5 years ago
Created attachment 832763 [details] [diff] [review]
part 1/2: v4

Sorry for all the serious mistakes in previous revision.  Should have a test before review.
Attachment #831561 - Attachment is obsolete: true
Attachment #832763 - Flags: review?(gene.lian)
(Assignee)

Comment 23

5 years ago
Created attachment 832765 [details] [diff] [review]
part 2/2: test cases

This patches add test cases for SMS only.
Attachment #832765 - Flags: review?(gene.lian)
(Assignee)

Comment 24

5 years ago
Created attachment 832767 [details] [diff] [review]
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.
(Reporter)

Comment 26

5 years ago
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
Attachment #832763 - Flags: review?(gene.lian) → review+
(Reporter)

Comment 27

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

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

Nice!
Attachment #832765 - Flags: review?(gene.lian) → review+
(Assignee)

Comment 28

5 years ago
Created attachment 832835 [details] [diff] [review]
part 1/2: v5

Address previous nits.
Attachment #832763 - Attachment is obsolete: true
Attachment #832835 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a7aa3ff58f78
https://hg.mozilla.org/mozilla-central/rev/e255bea5f2c1
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 942780
This will be part of 1.3 since it landed in gecko 28.
blocking-b2g: 1.3? → ---
You need to log in before you can comment on or make changes to this bug.