Closed Bug 982029 Opened 6 years ago Closed 5 years ago

[MMS/SMS] Display email in SMS/MMS Threads

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ta-matsuura, Assigned: na-matsumoto)

References

Details

Attachments

(1 file, 3 obsolete files)

For display SMS/MMS threads, email adress needs to be shown.

 [User story]
1. Run message app (some messages(sms/mms) with email address already recieved)
2. Go to the thread list.
3. Go into the thread.

[Eexpected]
- the subheader containing the email in the thread list[2] and the thread[3] both.
- in [2], we'll need to show the email if the receiver/senderis not a contact.
Blocks: 974333
Assignee: nobody → na-matsumoto
Summary: [MADAI][MMS/SMS] Display SMS/MMS Threads → [MADAI][MMS/SMS] Display email in SMS/MMS Threads
Summary: [MADAI][MMS/SMS] Display email in SMS/MMS Threads → [MMS/SMS] Display email in SMS/MMS Threads
Depends on: 974878
Target Milestone: --- → 2.0 S4 (20june)
Depends on: 974867
Blocks: 997547
Target Milestone: 2.0 S4 (20june) → ---
Attachment #8450890 - Flags: review?(schung)
Attachment #8450890 - Flags: review?(felash)
Hi, Julien and Steve.
I PR Attachment 8450890 [details].
Please review it.
I had a first look but I'm too tired to review properly. Will look at it monday.
Hi, Julien. Thanks for your answer.
How about the situation of the reviews?
Hi, Julien. Thanks for your answer.
How about the situation of the reviews?
I should be able to look at it today
Sorry, still couldn't today (lots of meetings). Tomorrow first thing...
Comment on attachment 8450890 [details] [review]
Display email in SMS/MMS Threads patch on github

commented on github

I didn't try on a device yet but I think you missed some locations and that the implementation for findByAddress is not correct.
Attachment #8450890 - Flags: review?(schung)
Attachment #8450890 - Flags: review?(felash)
Attachment #8453569 - Flags: review?(felash)
Hi, Julien. Thanks for your review.
I modified of your pointed out and rebased it.
Could you please review Attachment 8453569 [details]?
Flags: needinfo?(felash)
Comment on attachment 8453569 [details] [review]
Display email in SMS/MMS Threads patch on github

Sorry, I couldn't review completely but I've put comments on github so that you can work.

I'll also try on a device and report later today whether this works as expected.
Flags: needinfo?(felash)
Attachment #8450890 - Attachment is obsolete: true
Attachment #8453569 - Attachment is obsolete: true
Attachment #8453569 - Flags: review?(felash)
Attachment #8455148 - Flags: review?(felash)
Hi, Julien. Thanks for your review.
I modified of your pointed out and rebased it.
Could you please review Attachment 8455148 [details]?
Comment on attachment 8455148 [details] [review]
Display email in SMS/MMS Threads patch on github

It's really a good work !

I found 2 issues, one of them can be fixed now, the other can be fixed in a separate bug. And some minor nits otherwise.

Please request review again once you're ready. Also, it can be easier if you add a separate commit with your new changes, so that I can review it quicker.

Thanks a lot!
Attachment #8455148 - Flags: review?(felash)
Attachment #8455148 - Attachment is obsolete: true
Attachment #8456684 - Flags: review?(felash)
Hi, Julien. Thanks for your review.
I modified of your pointed out and rebased it.
And I corrected STR2.
STR1 is going to regist another Bug.

Could you please review Attachment 8456684 [details]?
Flags: needinfo?(felash)
Comment on attachment 8456684 [details] [review]
Display email in SMS/MMS Threads patch on github

r=me but please fix the latest nit I pointed out in the pull request.

Once you've fixed the nit, you can put "r=julien" in the commit log, then you can push the new version, keep r+ here, and put "checkin-needed" in the "keywords" section of the bug.

Thanks for your work !
Attachment #8456684 - Flags: review?(felash) → review+
Flags: needinfo?(felash)
I filed bug 1039531 for the remaining issue I found while reviewing.
Hi, Julien. Thanks for your review.
I modified of your pointed out.

Could you please review  it?
Flags: needinfo?(felash)
Keywords: checkin-needed
Now, I think STR1(bug 1039531) does not happen.
How about ?
I'll squash+merge myself.
Keywords: checkin-needed
(In reply to Naoya Matsumoto from comment #20)
> Now, I think STR1(bug 1039531) does not happen.
> How about ?

Looks like you're right :) Was probably fixed by the same fix than for STR2. I thought it was a separate bug because it was happening both for contacts and unknown recipients, but looks like I was wrong.

Thanks !
master: 09961a72e6651786d56876113673847b2f85015e
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(felash)
Resolution: --- → FIXED
[Blocking Requested - why for this release]:
Nominating this for v2.0, as desired on a 2.0 product.
blocking-b2g: --- → 2.0?
ni? Wayne on how we should be treating these late requests for 2.0.  Wayne, can you drive this bug offline?
Flags: needinfo?(wchang)
From a thread with Wayne and Lucas I can confirm that it is too late to accept this new feature work in 2.0. If this work is required for partner product, it will need to be cherry picked off of the 2.1 branch.
blocking-b2g: 2.0? → ---
Flags: needinfo?(wchang)
set partner flag requested by partner.
Group: kddi-confidential
Group: kddi-confidential
You need to log in before you can comment on or make changes to this bug.