992193 - [Dialer] USSD notify/request specific UI management

Status: VERIFIED FIXED

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

Description

[Noemí Freire (:noemi)]

Attachment: Current_USSD-notify_UI.png

Currently both types of USSD codes are handled very similar in terms of UI, so no matter if the network is waiting for a response (USSD request, please see "Current_USSD-request_UI.png) or not (USSD notify, please see "Current_USSD-notify_UI.png") we always show "Send" button in the header, in the first case "Send" button will be activated when typing something in the input field in the second case "Send" button will be always shown as deactivated.
The proposal would be to remove "Send" button for USSD notify case, so we will apply the most appropriated UI for each case. ni to UX to confirm the right path to be followed here.

Comment 1

[Noemí Freire (:noemi)]

Attachment: Current_USSD-request_UI.png

Comment 2

[Noemí Freire (:noemi)]

Adding to backlog to be properly prioritized. Thanks!

Comment 3

[Carrie Wang [New account: [e-mail]]]

Hi Noemi, 

Yes, I've faced this case before and felt quite confused. I agree with your proposal here. The send button in the case of USSD notify should be removed. Thanks!

Comment 4

[Fernando Campo (:fcampo)]

Attachment: Link to PR - https://github.com/mozilla-b2g/gaia/pull/19247

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 8422450 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19247

Hey, I know we don't have a mmi_ui_test.js yet, but I think we should be able to cover this change with unit test pretty cheaply.

And since we have some changes/refactorings coming to this code, we definitely should! :)

Comment 6

[Fernando Campo (:fcampo)]

(In reply to Etienne Segonzac (:etienne) from comment #5)
> Comment on attachment 8422450 [details] [review]
> Link to PR - https://github.com/mozilla-b2g/gaia/pull/19247
> 
> Hey, I know we don't have a mmi_ui_test.js yet, but I think we should be
> able to cover this change with unit test pretty cheaply.
> 
> And since we have some changes/refactorings coming to this code, we
> definitely should! :)
Thing is, mmi_test.js is using MockMmiUI (heavily dependance, actually), which doesn't let me test the calls to the real ui. So we have some options here:
- By creating a mmi_ui_tests.js I could test that the calls to the functions trigger the proper result, if that is enough at the moment.
- Other option (ideal) would be to use mmi_tests.js to test both functionality and UI changes, but that would take me some time, to stop using the mock and rely on the real_ui.

Having in mind that this is a small fix, I'd go for the first option, create a quick mmi_ui_test file and test that a mock_message received triggers the proper UI changes. WDYT Etienne?

Comment 7

[Etienne Segonzac (:etienne)]

(In reply to Fernando Campo (:fcampo) from comment #6)
> (In reply to Etienne Segonzac (:etienne) from comment #5)
> > Comment on attachment 8422450 [details] [review]
> > Link to PR - https://github.com/mozilla-b2g/gaia/pull/19247
> > 
> > Hey, I know we don't have a mmi_ui_test.js yet, but I think we should be
> > able to cover this change with unit test pretty cheaply.
> > 
> > And since we have some changes/refactorings coming to this code, we
> > definitely should! :)
> Thing is, mmi_test.js is using MockMmiUI (heavily dependance, actually),
> which doesn't let me test the calls to the real ui. So we have some options
> here:
> - By creating a mmi_ui_tests.js I could test that the calls to the functions
> trigger the proper result, if that is enough at the moment.
> - Other option (ideal) would be to use mmi_tests.js to test both
> functionality and UI changes, but that would take me some time, to stop
> using the mock and rely on the real_ui.
> 
> Having in mind that this is a small fix, I'd go for the first option, create
> a quick mmi_ui_test file and test that a mock_message received triggers the
> proper UI changes. WDYT Etienne?

Yeah the first option is exactly what I had in mind, we agree :)

Comment 8

[Fernando Campo (:fcampo)]

Having some problems with the test (postMessage and mocha).

After discussing on IRC with Julien, probably the best option is to upgrade mocha (bug 874510) instead of making workarounds in here.

Comment 9

[Etienne Segonzac (:etienne)]

Did you try doing a |new CustomEvent('message')|?

Comment 10

[Fernando Campo (:fcampo)]

(In reply to Etienne Segonzac (:etienne) from comment #9)
> Did you try doing a |new CustomEvent('message')|?
Yeah, tried customEvent, messageEvent after Julien's advice.
Also run the tests inside a eventListener (Rik's tip). 
Every time, same thing, as the tested action happens after a 'message' event received, and Mocha uses postMessage for the ticks, we receive a lot more events than we should, and test never goes complete :(

Comment 11

[Etienne Segonzac (:etienne)]

(In reply to Fernando Campo (:fcampo) from comment #10)
> (In reply to Etienne Segonzac (:etienne) from comment #9)
> > Did you try doing a |new CustomEvent('message')|?
> Yeah, tried customEvent, messageEvent after Julien's advice.
> Also run the tests inside a eventListener (Rik's tip). 
> Every time, same thing, as the tested action happens after a 'message' event
> received, and Mocha uses postMessage for the ticks, we receive a lot more
> events than we should, and test never goes complete :(

:/

New suggestion (from irc)
> and if you don't init the MmiUI and call handleEvent({message}) from the test ? :)

Comment 12

[Anthony Ricaud (:rik)]

This works:
    test('Response is hidden', function(done) {
      window.addEventListener('message', function(evt) {
        // Bug xyz - Mocha uses message event so we're filtering them out
        if (evt.data.type != 'mmi-success') {
          return;
        }
        assert.isTrue(MmiUI.hideResponseForm.called);
        assert.isTrue(MmiUI.sendNode.classList.contains('hide'));
        assert.isFalse(MmiUI.mmiScreen.classList.contains('responseForm'));
        done();
      });
    });

Comment 13

[Fernando Campo (:fcampo)]

Comment on attachment 8422450 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19247

Rik comments added, tests working (with comments inside, as we can't test everything yet).
Hopefully enough for landing while we wait for bug 874510

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8422450 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19247

kudos!

Comment 15

[Fernando Campo (:fcampo)]

merged on master - 1fe6e6f9518ae3cf57f374048955d209e9b797da

Thanks all for the help with the tests!

Comment 16

[Loli (:lolimartinezcr)]

Tested and working.
Hamachi
2.0
Gecko-1a316f5
Gaia-deed49c

- "Send" button not appear in ussd-notify case.
- "Send" button appear disabled in ussd-menú case and any option isn't selected.
- "Send" button appear enabled in ussd-menú case and any option is selected.