1000112 - Desktop client needs the ability to add a contact locally

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Comment 1

[Romain Testard [:RT]]

This affects the local copy of contacts only, we are not adding/updating contacts to the Google servers
Manually imported Google contacts and manually added contacts are 2 segregated lists. Managing duplicates and reconciliation of separate copies of the same contact on both lists is NOT required (we can end-up with 2 identical contacts on both lists).

Comment 2

[Romain Gauthier [:tOkeshu]]

We will start with the "backend" (backend here means the business logic part of the frontend application). And then add the UI on top of that.

For the persistence of contacts we will use IndexedDB. The bare API is not great, so I considered using abstractions like localForage[1]. But after discussing with :standard8 the API lacks of lookup and error handling features, so we will build our own abstraction for contacts.

Talkilla had a similar approach for contacts so I ported part of the code using promises[2] this time.
The branch is available on github[3].

[1] https://github.com/mozilla/localforage
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
[3] https://github.com/adamroach/gecko-dev/tree/bug-1000112-add-contact

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

At the time we considered db.js <https://github.com/aaronpowell/db.js/> for Talkilla, have you looked into it? Its promise based API looks very convenient, and the project seems active.

Comment 4

[Alexis Metaireau (:alexis)]

Would it make sense to actually make localforage go forward, specially given the fact that it's a mozilla supported project?

Comment 5

[Romain Testard [:RT]]

Regarding contacts management, the FFOS code allow adding contacts locally and also local import from Google.
Does it make sense to re-use the FFOS contacts application code in some places?

Comment 6

[Romain Testard [:RT]]

Borja, can you point us to where the FFOS code for contact management is?

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

The Contacts WebAPI code lives at [1]. I am not sure if it's available in Desktop though (ni? gwagner).

The Gmail importer code is indenpendant from the Contacts WebAPI and it lives in Gaia at [2].

[1] https://mxr.mozilla.org/mozilla-central/source/dom/contacts/
[2] https://mxr.mozilla.org/gaia/source/shared/js/contacts/import/

Comment 8

[Romain Gauthier [:tOkeshu]]

(In reply to Fernando Jiménez Moreno [:ferjm] (work week, not reading bugmail) from comment #7)
> The Contacts WebAPI code lives at [1]. I am not sure if it's available in
> Desktop though (ni? gwagner).

We already explored the Contacts WebAPI scenario in Talkilla and the outcome was to not use it on desktop.

> The Gmail importer code is indenpendant from the Contacts WebAPI and it
> lives in Gaia at [2].

Sure the Gmail importer will be useful regarding bug 972079.

> [1] https://mxr.mozilla.org/mozilla-central/source/dom/contacts/
> [2] https://mxr.mozilla.org/gaia/source/shared/js/contacts/import/

Comment 9

[Romain Gauthier [:tOkeshu]]

(In reply to Alexis Metaireau (:alexis) from comment #4)
> Would it make sense to actually make localforage go forward, specially given
> the fact that it's a mozilla supported project?

Well, we already discussed about that with :standard8 and localforage does fit well we our use case.
According to the documentation [1], localforage does not support lookups and proper error handling.

db.js [2] seems a way better candidate.

Mark, do you think we could use db.js in place of the current abstraction [3] ?

[1] http://mozilla.github.io/localForage/
[2] https://github.com/aaronpowell/db.js/
[4] https://github.com/adamroach/gecko-dev/tree/bug-1000112-add-contact

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

FWIW, I'd support using db.js.  That said, instead of starting with the backend, perhaps we could get a quick sketch from Darrin of intended UX here in order to help frame the backend thinking around the most important things?

Comment 11

[Darrin Henein [:darrin]]

Play with this: https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#contacts

You can add contact, edit contacts (click the green icon which appears on hover) and presumably remove contacts (local only). Read the notes on the right for clarification. Let me know if there are still further questions :)

Comment 12

[Mark Banner (:standard8)]

db.js seems a reasonable API and it is promised base which is a bonus.

My only slight concern is if we're going to need access from the background service, but lets design this based on the user stories, and work that out if we need to.

Comment 13

[:shell escalante]

this is blocked on the backing store, but also db.js is a wrapper around index.db. relies on origins (no authority to indicate when loading. similar to HAWK issue in Fx32 with cooking storing in prefs - library that's not part of mozilla.

sounds like we need investigation around ways to securely access.  SQL light possible, despite cumbersome interface.

Comment 14

[Adam Roach [:abr]]

To clarify -- the issue with using IndexedDB is that is uses a same-origin policy. While it is *possible* that this may work with the null origins that our pages load into, experience with things like cookies makes me think that this is extremely unlikely. Even if it worked, we would want to make sure that we don't have unintended side effects -- imagine the fiasco if data stored with a null origin could be accessed from other origins simply because it's an unexpected/untested value in the code somewhere.

I think the safe thing to do here is to step away from the web-accessible interfaces and use XPCOM-exposed APIs like OS.File (https://developer.mozilla.org/en-US/docs/JavaScript_OS.File) or Storage (https://developer.mozilla.org/en-US/docs/Storage). This does mean that we'll be adding surface to LoopServiceAPI for contact handling, but I think that's a good thing.

Comment 15

[Adam Roach [:abr]]

Another interesting observation that Fernando made: the Contacts API (https://developer.mozilla.org/en-US/docs/Web/API/Navigator.mozContacts) is available on Desktop now. There are some challenges here (how do we deal with making imported contacts immutable?), but it might make access easier.

Fernando also pointed out that he's seen IndexedDB used from chrome priv code on Desktop, so the null origin issue I'm worried about may not exist.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

See also <https://bugzilla.mozilla.org/show_bug.cgi?id=972079#c16>.

Re IndexedDB, I suspect that one of the things you (abr) said not to long ago is correct: our lives will likely be easier in the medium and long runs if someone takes another run at giving the desktop code a real origin.  My guess is that a b2g-style app:// origin is the next thing to try, and I'm happy to chat about my experiences if/when someone is available to pick that up.

Comment 17

[Romain Gauthier [:tOkeshu]]

So I worked on the possibility to use indexedDB with the help of :Standard8 and here is what I got:

- Using indexedDB via the chrome content from the panel (i.e proxyfied via the navigator.mozLoop API) was not very successful
- I got some help from :ttaubert to see if indexedDB could be used in the panel as it is currently used in about:home. He made a patch [1] I tested and can confirm with can used indexedDB in the panel with this.

The patch just adds about:looppanel and about:loopconversation in the whitelist origins for unprivileged content pages like about:home. The database is scoped to a specific origin shared between about:looppanel and about:loopconversation.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1028187

Comment 18

[Romain Gauthier [:tOkeshu]]

Here is the last version of my branch that should rely on Tim's patch [1]

[1] https://github.com/adamroach/gecko-dev/commit/d5b286d6f3d2749e7d0299bd1b72d3b9e933890e

Comment 19

[Maire Reavy [:mreavy]]

Hi Romain -- I need you to upload your patch against trunk today so we can assess how much work remains.  When you upload you patch, please provide a list/description of what work remains to finish and land it.  I realize it's blocked by Bug 1028187 (which is being worked by another developer while Tim is on PTO). Thanks.

Comment 20

[Romain Gauthier [:tOkeshu]]

Attachment: Implement database abstraction for contacts

This is a WIP patch. It only brings tested abstraction for contacts on top of IndexedDB.

We still have to ponder if we want to use db.js to improve the abstraction. The UI still need to be implemented also.

Comment 21

[Maire Reavy [:mreavy]]

Updated points per conversation with Mike

Comment 22

[:Paolo Amadini]

NiKo, do you confirm attachment 8455309 [details] [diff] [review] is obsolete and this bug is only related to the ReactJS UI work now?

Comment 23

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Yeah, it's a very old attempt at porting the old Talkilla code to Loop for contact handling. 

I don't know what the precise scope of this bug is; we could decide that it's only about the UI, the mutation parts being covered by bug 1038716.

Comment 24

[:Paolo Amadini]

Attachment: Test for CSS styles and animation

Comment 25

[:Paolo Amadini]

Attachment: Test for separate view without animation

The animation creates issues both with performance and keyboard interaction. This is a simpler version without any animation or effects.

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch: WIP based on Paolo's work, including styling

Comment 27

[:Paolo Amadini]

Attachment: WIP

We should use HTML5 validation for e-mail addresses.

This removes over 130 lines by simplifying the panel CSS.

Comment 28

[:Paolo Amadini]

Attachment: Initial version

Comment 29

[:Paolo Amadini]

Attachment: Version for review

Comment 30

[Mike de Boer [:mikedeboer]]

Comment on attachment 8495298 [details] [diff] [review]
Version for review

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

Looks good Paolo! So I'm requesting review from Nicolas, because I'd like to see the CSS 'refactor' and the markup changes for the Share panel validated.

I hope you don't mind.

::: browser/components/loop/MozLoopAPI.jsm
@@ +541,5 @@
>        value: function(histogramId, value) {
>          Services.telemetry.getHistogramById(histogramId).add(value);
>        }
>      },
> +  

OMG! Whitespace! Kill it! With fire!

::: browser/components/loop/content/js/panel.jsx
@@ +49,5 @@
> +          tabButtons.push(
> +            <li className={cx({selected: isSelected})}
> +                key={i}
> +                data-tab-name={tabName}
> +                onClick={this.handleSelectTab}>

Can you go ahead and change this to a self-closing tag? Something I forgot earlier...

Comment 31

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8495298 [details] [diff] [review]
Version for review

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

Looks good! Debatable nits here and there you may want to address. I was surprised not seeing any test here, I suppose they'll be added in a part 2… or are the existing one covering everything here already?

Also, I'd really like to see the contacts stuff being integrated into the UI component showcase, so we could ensure that everything keeps consistent regarding the CSS changes — a part 2 would be warmly welcome ;)

rs=me (I didn't build to test, I wanted to provide really fast feedback here, mostly with the code.)

::: browser/components/loop/content/js/contacts.jsx
@@ +192,5 @@
> +            {shownContacts.available ?
> +              shownContacts.available.sort(this.sortContacts).map(viewForItem) :
> +              null}
> +            {shownContacts.blocked ?
> +              <h3 className="contact-separator">{mozL10n.get("contacts_blocked_contacts")}</h3> :

Nit: Unsure, but is inserting an <h3> in the middle of a list "valid" in html5? This is quite possibly bikeshedding, but I usually like keeping valid semantics. Your call.

@@ +226,5 @@
> +        pristine: true,
> +        name: "",
> +        email: "",
> +      });
> +    },

Nit: You could merge with getInitialState return value to avoid maintaining two distinct default objects:

initForm: function(contact) {
  this.setState(_.extend(this.getInitialState(), {
    contact: contact || null
  }));
},

Object.assign would work as well if we want to use ES6 here — though it's not available in beta atm, but it's probably not an issue for this bug targeted release. Your call :-)

@@ +261,5 @@
> +            }],
> +            category: ["local"]
> +          }, err => {
> +            if (err) {
> +              throw err;

Nit: Add an XXX comment for proper end user notification (needs UX I guess).

@@ +280,5 @@
> +          <div className="header">{mozL10n.get("add_contact_button")}</div>
> +          <label>{mozL10n.get("edit_contact_name_label")}</label>
> +          <input ref="name" required pattern="\s*\S.*"
> +                 className={cx({pristine: this.state.pristine})}
> +                 valueLink={this.linkState("name")} />

Nit: I usually wrap my form inputs in a <label/> tag, so clicking on the label focuses the field without having to define a `for` attribute; whatever your choice is here, making labels clickable is often considered as good UX/a11y practice. Your call.

::: browser/components/loop/content/js/panel.jsx
@@ +41,5 @@
>      render: function() {
>        var cx = React.addons.classSet;
>        var tabButtons = [];
>        var tabs = [];
>        React.Children.forEach(this.props.children, function(tab, i) {

Nit: I could see a map here, eg. var tabs = React.Children.map(this.props.children… Your call.

@@ -398,3 @@
>        });
>        return (
> -        <PanelLayout summary={__("share_link_header_text")}>

I think PanelLayout is no more used, you should then remove it completely (and all references to it).

@@ +379,4 @@
>        });
>        return (
> +        <div className="generate-url">
> +          <div className="header">{__("share_link_header_text")}</div>

Nit: How about using a <header/> tag? Your call.

@@ +456,5 @@
>        this.setState({userProfile: navigator.mozLoop.userProfile});
>      },
>  
> +    startForm: function(name, contact) {
> +      this.refs[name].initForm(contact);

Interesting. This ties this component method to one of its children implementation, which is not the way I'd have done it. But if it works and doesn't raise any concern, we're good to go!

::: browser/components/loop/content/shared/js/views.jsx
@@ +680,5 @@
>  
> +  var Button = React.createClass({
> +    propTypes: {
> +      caption: React.PropTypes.string.isRequired,
> +      onClick: React.PropTypes.func.isRequired,

Shouldn't be required, for eg. when you want to use a button as a submit one (which is the default).

@@ +681,5 @@
> +  var Button = React.createClass({
> +    propTypes: {
> +      caption: React.PropTypes.string.isRequired,
> +      onClick: React.PropTypes.func.isRequired,
> +      disabled: React.PropTypes.bool,

Maybe we want to define getDefaultProps here, eg. to set disabled to false by default. Not required, but I'm finding this declarative style explicit & helpful.

@@ +688,5 @@
> +
> +    render: function() {
> +      var cx = React.addons.classSet;
> +      var classObject = { button: true, disabled: this.props.disabled };
> +      if (this.props.additionalClass) {

Nit: You should have a look at transferPropsTo http://facebook.github.io/react/docs/component-api.html#transferpropsto

Comment 32

[:Paolo Amadini]

Attachment: Updated patch

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #31)
> Review of attachment 8495298 [details] [diff] [review]:

Thanks for the valuable feedback! I've made some of the changes in the attached patch, and started a tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=af616e2b947d

> Looks good! Debatable nits here and there you may want to address. I was
> surprised not seeing any test here, I suppose they'll be added in a part 2…
> or are the existing one covering everything here already?

Filed bug 1073468.

> Also, I'd really like to see the contacts stuff being integrated into the UI
> component showcase, so we could ensure that everything keeps consistent
> regarding the CSS changes — a part 2 would be warmly welcome ;)

Filed bug 1073469.

> @@ +261,5 @@
> > +            }],
> > +            category: ["local"]
> > +          }, err => {
> > +            if (err) {
> > +              throw err;
> 
> Nit: Add an XXX comment for proper end user notification (needs UX I guess).

Filed bug 1073472 for investigation. I think this just reports to the Console, which can be fine for local contacts.

> Nit: I usually wrap my form inputs in a <label/> tag, so clicking on the
> label focuses the field without having to define a `for` attribute; whatever
> your choice is here, making labels clickable is often considered as good
> UX/a11y practice. Your call.

I believe this just isn't the convention on Desktop, and doesn't happen elsewhere in the panel. We may want to change this, but I see this as something for a later UX polish phase.

> Nit: I could see a map here, eg. var tabs =
> React.Children.map(this.props.children… Your call.

Since were handling two output arrays at the same time, I think the current code is clearer.

> I think PanelLayout is no more used, you should then remove it completely
> (and all references to it).

I couldn't find any other reference in browser/components/loop.

> @@ +456,5 @@
> > +    startForm: function(name, contact) {
> > +      this.refs[name].initForm(contact);
> 
> Interesting. This ties this component method to one of its children
> implementation, which is not the way I'd have done it. But if it works and
> doesn't raise any concern, we're good to go!

I'll go ahead with this, but would of course be interested in hearing about possibly better solutions.

> ::: browser/components/loop/content/shared/js/views.jsx
> > +      onClick: React.PropTypes.func.isRequired,
> 
> Shouldn't be required, for eg. when you want to use a button as a submit one
> (which is the default).

Plain submit buttons just break the panel, by navigating away :-/ If we find a solution, maybe using real form tags, we can revisit this.

> @@ +688,5 @@
> > +
> > +    render: function() {
> > +      var cx = React.addons.classSet;
> > +      var classObject = { button: true, disabled: this.props.disabled };
> > +      if (this.props.additionalClass) {
> 
> Nit: You should have a look at transferPropsTo
> http://facebook.github.io/react/docs/component-api.html#transferpropsto

I'm not sure about how I could apply this API to what I'm trying to do here.

Comment 33

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to :Paolo Amadini from comment #32)

I don't want the comments below to block you from landing this, so up to you to address them.

> > Interesting. This ties this component method to one of its children
> > implementation, which is not the way I'd have done it. But if it works and
> > doesn't raise any concern, we're good to go!
> I'll go ahead with this, but would of course be interested in hearing about
> possibly better solutions.

Well, the form component could be passed the contact object as a prop, so that it could:
- prefill fields with contact properties locally, and allow editing them
- on form submit, call some function (passed as a prop) to notify the parent (or any listener, eg. a future store) that new data is available for this contact

I could try to update the patch to provide a sample implementation if you want.

> Plain submit buttons just break the panel, by navigating away :-/ If we find
> a solution, maybe using real form tags, we can revisit this.

Yeah, forms have this convenient submit event we can listen to and prevent defaults to prevent actual navigation… This is a common pattern in React:

handleFormSubmit: function(event) {
  event.preventDefault();
  var data = this.refs.field.getDOMNode().value;
}

<form onSubmit="this.handleFormSubmit">
  <input ref="field" />
  <button>submit me</button>
</form>

I keep thinking that's what we should be using here, but no big deal.

> > @@ +688,5 @@
> > > +
> > > +    render: function() {
> > > +      var cx = React.addons.classSet;
> > > +      var classObject = { button: true, disabled: this.props.disabled };
> > > +      if (this.props.additionalClass) {
> >
> > Nit: You should have a look at transferPropsTo
> > http://facebook.github.io/react/docs/component-api.html#transferpropsto
>
> I'm not sure about how I could apply this API to what I'm trying to do here.

Transfering the className props to the button element, so no need of `additionalClass`:

var MyButton = React.createClass({
  render: function() {
    return this.transferPropsTo(<button/>);
  }
});

<MyButton className="foo"/> // -> <button class="foo">

Comment 34

[:Paolo Amadini]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #33)
> Well, the form component could be passed the contact object as a prop, so
> that it could:
> - prefill fields with contact properties locally, and allow editing them

Hm, I need some more context to understand how this is related to the original observation of the parent calling into a function defined on the child.

For the add case, we don't have a contact. How would this remove the need for the function call?

For the edit contact case, the form reset should only be done once, when the form is opened. That is like setting a state at a given moment in time, rather than inheriting state from the parent.

As for having the currently editing contact as a prop, it might be feasible, but I guess we need to store it as state on the container as well (to be used by the render method), and clear it when editing is finished (to avoid leaks or stale references).

> - on form submit, call some function (passed as a prop) to notify the parent
> (or any listener, eg. a future store) that new data is available for this
> contact

I think this is a separate thing. This feedback loop is currently handled by the contacts API, that notifies the interested components (not necessarily the direct parent).

> I could try to update the patch to provide a sample implementation if you
> want.

If you could create a demo diff on top of the current patch, it would be neat! I could apply the pattern to future updates to the code.

> handleFormSubmit: function(event) {
>   event.preventDefault();
>   var data = this.refs.field.getDOMNode().value;
> }
> 
> <form onSubmit="this.handleFormSubmit">
>   <input ref="field" />
>   <button>submit me</button>
> </form>
> 
> I keep thinking that's what we should be using here, but no big deal.

Looks good to me, definitely in the list for a future update.

> Transfering the className props to the button element, so no need of
> `additionalClass`:
> 
> var MyButton = React.createClass({
>   render: function() {
>     return this.transferPropsTo(<button/>);
>   }
> });
> 
> <MyButton className="foo"/> // -> <button class="foo">

But I also need to merge it with additional classes.

Comment 35

[:Paolo Amadini]

https://tbpl.mozilla.org/?tree=Try&rev=7f2300c47c3e

https://hg.mozilla.org/integration/fx-team/rev/a7e7c1333d68

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a7e7c1333d68

Comment 37

[u279076]

I've not discovered a way to test this in the latest Nightly. Can someone please provide clear steps?

Paul, once clarification has been given, please test this in the latest Nightly and the Try-server build here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9eb2cbac352

Comment 38

[:Paolo Amadini]

You will be able to add contacts when bug 1038257 is resolved.

Comment 39

[u279076]

(In reply to :Paolo Amadini from comment #38)
> You will be able to add contacts when bug 1038257 is resolved.

Dropping this from the fig verifications since testing is blocked on bug 1038257. We'll want to still verify this post-uplift.

Comment 40

[Paul Silaghi, QA [:pauly]]

I'm able to add contacts even if I'm not signed in. Is this expected for now?

Comment 41

[:Paolo Amadini]

(In reply to Paul Silaghi, QA [:pauly] from comment #40)
> I'm able to add contacts even if I'm not signed in. Is this expected for now?

Yes, these are kept as local contacts.

Comment 42

[u279076]

Thanks for your help, Paul. I'm marking this verified fixed based on your testing. Also flagging this for a Moztrap test.

Comment 43

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8495884 [details] [diff] [review]
Updated patch

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 44

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e7ef7666582

Comment 45

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8495884 [details] [diff] [review]
Updated patch

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.

Comment 46

[u279076]

Paul, this has been uplifted to Aurora. Can you please test this to confirm it's working as expected?

Comment 47

[Paul Silaghi, QA [:pauly]]

Verified fixed 34.0a2 (2014-10-13) Win 7