838005 - [SMS][User Story] Browser invocation from message

Status: RESOLVED FIXED

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

Description

[Peter Dolanjski [:pdol]]

UCID: Messages-005

User Story:
As a user, I want the ability to launch the browser using a url in a message so that I don't need to remember the url to manually enter it into the Browser application.

Comment 2

[Casey Yee]

UX draft here for comment:
https://www.dropbox.com/s/0f3gpv2rkqupeq6/Messaging-Email%2BBrowserLinks.pdf

This one should be fairly straightforward.

The only issue I see here is that there is no easy way for the user to return to the messaging from the browser after clicking a link.

The compose email back button should return you back to messaging if the user decides to back-out from the email.

Comment 3

[satender]

Attachment: pull request for browser invocation

Comment 4

[satender]

Comment on attachment 721606 [details] [review]
pull request for browser invocation

https://github.com/mozilla-b2g/gaia/pull/8487

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 721606 [details] [review]
pull request for browser invocation

I'll let Borja review this one. I wonder if we should not put the url detection in /shared.

Comment 6

[satender]

sure, will move url logic to separate url_helper in /shared directory.

Comment 7

[Maria Angeles Oteo (:oteo)]

Attachment: Acceptance criteria for US 838005

Comment 8

[Maria Angeles Oteo (:oteo)]

(In reply to Casey Yee [:cyee] from comment #2)

> 
> The only issue I see here is that there is no easy way for the user to
> return to the messaging from the browser after clicking a link.
> 

It's out of the scope of the US, for the time being we can live without coming back to the SMS app after launching the browser

Comment 9

[Maria Angeles Oteo (:oteo)]

Attachment: Acceptance criteria for US 838005

Comment 10

[Maria Angeles Oteo (:oteo)]

One important thing to highlight: 
In case we have several clickable items in a single bubble, evey item should have an indepedent clickable area.

Comment 11

[Borja Salguero [:borjasalguero]]

Please squash your changes and add the reviewer name! Let's move to the next pull request. Thanks! Gracias! ;)

Comment 12

[ayman maat :maat]

Attachment: HTML5_SMS-MMSUserStorySpecifications_20130315_V1.0

Wireframe release:
HTML5_SMS-MMSUserStorySpecifications_20130315_V1.0

**new wireframes**
- SMS with phone number
- Phone number long press options
- Phone number not in contact list options
- SMS with URL
- SMS with email
- Email long press options

**updated wireframes**
- none

**deleted wireframes**
- none


to address this bug specifically refer to page: 8

Comment 13

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 0b67dbf9f7c98d02f5b9caa2d653398833a37a51
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 14

[Maria Angeles Oteo (:oteo)]

HI Julien,
what's the reason for not uplifing it? 
It was agreed that only MMS specific US will land in master but the SMS ones (838000, 838002, 838003, 838004 and 838005) can be merged in v1-train

Comment 15

[Julien Wajsberg [:julienw]]

Maria, do as you think is right.

I've seen the specification for this is named "HTML5_SMS-MMSUserStorySpecifications_20130315_V1.0" and I therefore supposed it would be better to _not_ uplift this until we've done everything. I don't want problems like the one in Bug 848778 to happen again.

If you feel good about this change, then I see no problem uplifting this -- as long as the dev helps with the conflict, of course.

Comment 16

[satender]

(In reply to John Ford [:jhford] from comment #13)

Hi John,
This is just a merge conflict. I don't have the commit rights yet to resolve this, Borja has been helping me in merging my branch change to master.

I did do a local cherry pick on my box, and its purely a case of merge conflict.

Hi Borja,
There are 2 files in the conflict, below is the changes which need to be done to do the merge conflict resolution:
sms/js/startup.js

var lazyLoadFiles = [
  'shared/js/async_storage.js',
  'shared/js/l10n_date.js',
  'shared/js/custom_dialog.js',
  'shared/js/desktop.js',
  'shared/js/notification_helper.js',
  'js/blacklist.js',
  'js/contacts.js',
  'js/message_manager.js',
  'js/thread_list_ui.js',
  'js/thread_ui.js',
  'js/waiting_screen.js',
  'js/utils.js',
  'js/search_utils.js',
  'js/fixed_header.js',
<<<<<<< HEAD
=======
  'js/link_helper.js',
  'js/action_menu.js',
  'js/link_action_handler.js',
>>>>>>> 0b67dbf... Merge pull request #8487 from ssaroha/sms-url-838005
  'shared/style/input_areas.css',
  'shared/style/switches.css',
  'shared/style/confirm.css',
  'shared/style_unstable/progress_activity.css',
  'style/custom_dialog.css'
];

removing following line numbers 21,22
<<<<<<< HEAD
=======

and following line 26 

>>>>>>> 0b67dbf... Merge pull request #8487 from ssaroha/sms-url-838005

will resolve the conflict here.


The second file which shows conflict is:

sms/js/thread_ui.js:

<<<<<<< HEAD
=======
    if (message.delivery === 'error')
      ThreadUI.addResendHandler(message, messageDOM);

    var bodyHTML = LinkHelper.searchAndLinkClickableData(bodyText);
    // check for messageDOM paragraph element to assign linked message html
    // For now keeping the containing anchor markup as this
    // structure is part of building blocks.
    // http://buildingfirefoxos.com/building-blocks/lists/
    // Todo: Open bug to fix contaning anchor to div to avoid
    // below extra innerHTML call
    var pElement = messageDOM.querySelector('p');
    pElement.innerHTML = bodyHTML;
    return messageDOM;
  },

  appendMessage: function thui_appendMessage(message, hidden) {
    // build messageDOM adding the links
    var messageDOM = this.buildMessageDOM(message, hidden);
    var timestamp = message.timestamp.getTime();
    messageDOM.dataset.timestamp = timestamp;
>>>>>>> 0b67dbf... Merge pull request #8487 from ssaroha/sms-url-838005

In the merged file, please remove the following lines 562, 563:
<<<<<<< HEAD
=======

and following line number 584
>>>>>>> 0b67dbf... Merge pull request #8487 from ssaroha/sms-url-838005

from the merged file.

Thanks

Comment 17

[Julien Wajsberg [:julienw]]

satender> you can resolve the conflict on your own repository and put the commit/branch url here. John can then fetch it and apply it on mozilla's repository.

Comment 18

[satender]

Hi Julien,
Here is the commit url:
https://github.com/ssaroha/gaia/commit/955f604d10ed72e487969fb874c6347260aab31e

I have tested the feature with the merged change on v1-train. From, the functionality perspective the one caveat is that browser app should have been used independently, before the user clicks on a url from within the sms message which triggers the url activity, otherwise the browser is not able to open the url with error message "Error: "TypeError: tab is null" {file: "app://browser.gaiamobile.org/js/browser.js" line: 1191}]".

This issue is there only on the v1-train, but is not there on the master. I am not able to figure out which particular bug number fixed this issue on master (otherwise I would have made that one as pre-requisite for this bug)

The above commit url resolves all the merge conflicts, they turned out to be a few more than what i had mentioned in comment #16.

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to satender from comment #18)

> This issue is there only on the v1-train, but is not there on the master. I
> am not able to figure out which particular bug number fixed this issue on
> master (otherwise I would have made that one as pre-requisite for this bug)


This is a very serious bug imho.

Ben, do you have an idea of what fixed that ?

Comment 20

[Ben Francis [:benfrancis]]

This sounds like bug 849917

Comment 21

[satender]

John,
please apply the following commit url: https://github.com/ssaroha/gaia/commit/955f604d10ed72e487969fb874c6347260aab31e

I have tested it on v1-train locally, and it works with the latest browser fix 849917 in v1-train.

Comment 22

[Julien Wajsberg [:julienw]]

So, the bug in comment 18 is related to the Browser and not to this patch, so let's go uplifting this !

Comment 23

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 303fceefa90aa31bd88b7d983bc71a31e368514a

Comment 24

[Jason Smith [:jsmith]]

This might be TEF or Taipei owning testing here.

Tony - Can you find out who owns this user story?

Comment 25

[Maria Angeles Oteo (:oteo)]

The testing of this US owns to TEF team, they are preparing the test plan for it