990058 - [FTU-Separation] Re-implement Basket newsletter functionality in the separated FTU

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Fernando Campo (:fcampo)]

Right now we have FTU and Dialer linked through the newsletter sending at the end of the FTU.

> Reminder: If the user introduces an email address in the last screen of the FTU, this information (together with country and language) is sent to the mozilla basket service to subscribe the user to news related. If this process fails (e.g. no network), the FTU saves the information and will retry to send it when the Dialer app is opened.

We need to move the whole thing to shared folder, so it can be used from FTU and Dialer (or other app if we decide to change this) without risks.

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

It not just moving it to shared folder, we will need to make two different apps to communicate with each other.

Also we should think about this, should be the dialer now in charge of doing this?

If so, how are we going to communicate this, via IAC? DS? Flagging Etienne for his PoV here.

Comment 2

[Etienne Segonzac (:etienne)]

How about storing the email in a "certified only" datastore, and having the system app do the http post once online?

Comment 3

[Fernando Campo (:fcampo)]

+1 to Etienne's, I'd rather have system dealing with it than waiting till a specific app is opened.

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

Right now, datastore is just for certified so wont be a problem, but will need to take into account when datastore will be available by privileged (soon).

Now we just need to convince a system peer :)

But definitely Etienne's idea looks like the best solution.

Comment 5

[Fernando Campo (:fcampo)]

(In reply to Francisco Jordano [:arcturus] from comment #4)
> Now we just need to convince a system peer :)
What a coincidence...Etienne is a system's peer :p 
What do you need for getting convinced?

Bad news is, I'll be on PTO from tomorrow till 8th, so won't start with this till then. Will let it unassigned in case someone wants to do it.

Comment 6

[Fernando Campo (:fcampo)]

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

Asking Francisco for the FTU part, and Etienne for System

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

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

Hi Fernando,

thanks for this patch, we will need also a system peer reviewer (unless Etienne is system peer).

Left my comments on github, just r- the patch cause there are two main things that are a bit weird to me:
- The owner of this datastore is communications, I though it will be leaving in the FTU app.
- Didn't see where in the FTU are we saving the email to the datastore. Is that code already in working? If so, against which DS?
- We will need write permissions from the system to write in the DS to mark we already sent the email.

Comment 8

[Francisco Jordano [:arcturus] [:francisco]]

Thanks for the clarification on the |readOnly=false| meaning write permissions.

Also as we sync offline is better to wait till the FTU is separated to add all the changes together, IMHO, since installing an app with a DS and then having the same DS name in another app could be tricky.

Thanks a lot Fernando!

Comment 9

[Fernando Campo (:fcampo)]

After talking with Francisco, and clarify the datastore-ownership problem, we decided to wait till the FTU full split is merged (bug 1010782), and then modify this so the ownership of the datastore will be properly addressed. 
Even if we lose the newsletter functionality for some time (till this bug is merged), is preferably than duplicating datastores, or have a mess with ownerships.

(In reply to Francisco Jordano [:arcturus] from comment #7)
> Comment on attachment 8425519 [details] [review]
> Link to PR - https://github.com/mozilla-b2g/gaia/pull/19444
> 
> Hi Fernando,
> 
> thanks for this patch, we will need also a system peer reviewer (unless
> Etienne is system peer).
I thought he was. If not, who should I ask for review?

Comment 10

[Fernando Campo (:fcampo)]

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

PR updated after landing bug 1010782 with ftu as a separated app \o/

Comment 11

[Fernando Campo (:fcampo)]

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

f? to Jose Manuel so he is aware of the changes included, for the FTU separation mega task.

Comment 12

[Fernando Campo (:fcampo)]

(In reply to Fernando Campo (:fcampo) from comment #11)
> Comment on attachment 8425519 [details] [review]
> Link to PR - https://github.com/mozilla-b2g/gaia/pull/19444
> 
> f? to Jose Manuel so he is aware of the changes included, for the FTU
> separation mega task.
That was supposed to be META, but mega sounds much better tbh :D

Comment 13

[Jose Manuel Cantera]

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

looks doog although I left some comments on GH for your consideration

Comment 14

[Francisco Jordano [:arcturus] [:francisco]]

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

Second round looking much better, giving the r+ but would like to see the change related to being sure that we work with the correct datastore via store.owner verification against FTU manifest url.

Checked the code in the phone, both states, without connectivity and the value stored in the DS, and then with connectivity and after a while, value changed to send.

Great work, r+ to the FTU side.

Thanks Fernando!

Comment 15

[Etienne Segonzac (:etienne)]

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

This is shaping up nicely! My comments are on github (mainly on tests).

Comment 16

[Fernando Campo (:fcampo)]

I've left a comment on github related to the change from timeout to fakeTimers, as I didn't manage to make it work properly (also kudos to :Rik and :julienw that were helping me on testing promises properly).

Please check whether I have something wrong in my code and fakeTimers can work it out, or we should keep using timeouts.

Comment 17

[Etienne Segonzac (:etienne)]

Ok ok, there was a misunderstanding, and it was entirely my fault.
I saw timeouts in a test file so I said useFakeTimers! Because I always do that.

But you're not trying to test setTimeouts in your code (or use timeout based mocks, which is what I originally thought), you're trying to test promise-based code.
And you won't be able to do that if the method under test doesn't return a promise itself so you can chain the assertions.

To prevent any other misunderstanding here's an example.

The sendNewsLetter method needs to look like that:
```
  sendNewsletter: function(emailAddress) {
    return new Promise(function(resolve, reject) {
      LazyLoader.load('/shared/js/basket_client.js', function basketLoaded() {
        Basket.send(emailAddress, function itemSent(err, data) {
          if (err) {
            reject(err);
            console.error('Error sending data: ' + err);
            return;
          }

          if (data && data.status === 'ok') {
            // Once is sent, we update the DataStore
            Basket.getDataStore().then(function gotDS(store) {
              var newObj = {
                'emailSent': true
              };
              store.put(newObj, 1).then(resolve, reject);
            }).catch(function error(err) {
              console.error('Something went wrong: ' + err);
              reject(err);
            });
          }
        });
      });
    });
  }

```

If we want to be able to write tests like this one:
```
    suite('Sending the info >', function() {
      var updatedEmail = {
        'emailSent': true
      };

      setup(function() {
        sinon.spy(MockDatastore, 'put');
      });

      test('Datastore updated when email sent >', function(done) {
        NewsletterManager.sendNewsletter(email).then(function() {
          sinon.assert.calledWith(MockDatastore.put, updatedEmail);
          done();
        }, done);
      });
    });
```

Sorry again for the lost time caused by my useFakeTimers reflex :/

Comment 18

[Fernando Campo (:fcampo)]

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

Updated with Etienne's suggestions, use Promises for all the cases (even if we don't need an answer, it's better for testing)

Comment 19

[Etienne Segonzac (:etienne)]

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

Wonderful!

Is QA equipped to give this a spin once it lands?
(since it's not a user-facing feature)

Comment 20

[Fernando Campo (:fcampo)]

\o/ merged on master - 5a3810d83e6cf80e99457f6cad1992bfbd7d2401

thanks all for your help and sorry for the misunderstanding and the time it took to land.

About QA, we might need help from Tony, per comment 46 on bug 949170, so ni? some people about this.

Also, we shouldn't forget about pending related bug 968423

Comment 21

[Maria Angeles Oteo (:oteo)]

Including verifyme per Fernando's last comment

Comment 22

[Jason Smith [:jsmith]]

Okay - nkot - Can you have someone do some exploratory testing around this patch following the testing strategy we did in https://bugzilla.mozilla.org/show_bug.cgi?id=949170#c47?

Comment 23

[Natalya Kot [:nkot]]

(In reply to Jason Smith [:jsmith] from comment #22)
> Okay - nkot - Can you have someone do some exploratory testing around this
> patch following the testing strategy we did in
> https://bugzilla.mozilla.org/show_bug.cgi?id=949170#c47?

Peter will look into this today.

Comment 24

[Tony Chung [:tchung]]

looks like peter will own this.  removing ni? me.

Comment 25

[Peter Bylenga [:PBylenga]]

Fernando,

What is needed to test this in completion?  Who can verify changes on the server side that the emails are being added to the subscription list? Can QA have access to the server side to do it ourselves?   

Looking at the patch, I see console errors setup for failures although none setup for success.  Is there a way I can pull the variable that is being set to true when we receive the success response?

Comment 26

[Fernando Campo (:fcampo)]

(In reply to Peter Bylenga from comment #25)
> Fernando,
> 
> What is needed to test this in completion?  Who can verify changes on the
> server side that the emails are being added to the subscription list? Can QA
> have access to the server side to do it ourselves?
Access to the server would be nice to check if the email is received, but is not a must to test the patch. From the gaia side we can test that the server answers with a '200 OK' state.
From TEF we don't have any kind of access to that server, as it's part of Mozilla Basket services, maybe someone on their side can help.

> 
> Looking at the patch, I see console errors setup for failures although none
> setup for success.  Is there a way I can pull the variable that is being set
> to true when we receive the success response?
You can add console logs to the patch for success state, but I wouldn't like to have them on the final code to not pollute the console. Sadly I don't think we can pull any data from the DataStore without special permissions, but maybe I'm wrong.

The way the patch should work is:
- During FTU, on the last screen we add an email address on the input, and when we press Next, the system checks for connection:
  - If we have connectivity, we send the email to basket.
  - If we don't have connection, we store the address on a DataStore.

- System app has a timeout going on, waiting for 10 seconds of idle time, and starting the followign process:
  - If the FTU is running at the moment, we cancel the process.
  - If it's not, we check the mentioned DataStore for an email. 
    - If there's none, or the content of it has the 'already-sent' flag activated, we exit.
    - If we find an email, we look again for available connection, and send it if we have.
      - If we don't have connectivity, we create an observer to wait for onLine event that sends the email.

- After the email is sent, and we receive the '200 OK' response from server, we update the DataStore to flag the email as 'already-sent', so the process is not repeated again.

- This is done again everytime the system is rebooted.


Hope this helps

Comment 27

[Peter Bylenga [:PBylenga]]

I was able to verify that the user is signed up for Firefox OS Owner newsletter.  After signing up on my device, I signed up for a separate newsletter at https://www.mozilla.org/en-US/newsletter/ then when I got the confirmation mail for that newsletter I clicked on "modify my preferences" and was directed to a page where it lists all newsletters I was subscribed to.

I attempted to use https://gist.github.com/jrgm/4490638 to enable HTTP logging on my device but was unable to detect the message in the log when running through the STRs.

Environmental Variables:
Device: Flame
Build ID: 20140529040201
Gaia: 4142df90c71abdc1e3a87cd37dff1a22d5e38b34
Gecko: 1e712b724d17
Version: 32.0a1 (2.0) 
Firmware Version: v10G-2

Comment 28

[Peter Bylenga [:PBylenga]]

Attachment: HTTP_log.txt