Show fields for phone, email, address and groups by default

[xh3n1]

Issue #253

[codecov-io]

Codecov Report

Merging #259 into master will increase coverage by 0.28%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
+ Coverage   14.87%   15.16%   +0.28%     
==========================================
  Files          55       52       -3     
  Lines        1217     1174      -43     
==========================================
- Hits          181      178       -3     
+ Misses       1036      996      -40

Impacted Files	Coverage Δ
js/services/contact_service.js	0.66% <0%> (-0.02%)	⬇️
...ts/newContactButton/newContactButton_controller.js	7.69% <0%> (-0.65%)	⬇️
js/components/avatar/avatar_directive.js	6.25% <0%> (-0.9%)	⬇️
js/services/import_service.js
...components/importScreen/importScreen_controller.js
.../components/importScreen/importScreen_directive.js
...s/components/contactList/contactList_controller.js	1.76% <0%> (+0.04%)	⬆️
...omponents/contactImport/contactImport_directive.js	5.88% <0%> (+2.94%)	⬆️
...mponents/contactImport/contactImport_controller.js	33.33% <0%> (+27.08%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bebe03...3612d0c. Read the comment docs.

[MorrisJobke]

Hi @xh3n1 two little remarks:

1. could you revert the changes in js/dav/dav.js because they change the whitespace on over 1000 lines of code 😉 (and nothing else)
2. if you say in the comment "fixes #NUMBER" then github automatically closes this linked issue once the PR gets merged 😉 (see https://help.github.com/articles/closing-issues-using-keywords/ for details)

[nickvergessen]

See https://help.github.com/articles/closing-issues-using-keywords/ if you hate the word fixes ;)

[georgehrke]

This will add the three properties to any contact you view, not only to new ones.

Not sure that's the expected behavior.

Furthermore the pre-selected group issue is not fixed by this.

The expected behavior for the groups is that if you have selected a group in the left sidebar and click "New contact", the new contact is automatically added to the selected group.

[georgehrke]

@jancborchardt

[jancborchardt]

@georgehrke yes that is the expected behavior. :) As these are the 3 most common properties. It's also the behavior of Android and iOS to always have one empty field for those.

[skjnldsv]

@jancborchardt the field isn't saved empty. @xh3n1'commit will add empty ones in the vcard.
On my android the fields are displayed empty, but nothing is saved on the vcard until I write somthing into those fields.

[jancborchardt]

Right, of course it shouldn’t be saved empty, just displayed empty. A virtual field if you wish. ;)

[xh3n1]

@skjnldsv I'm not sure I understand what you are saying, However I think a solution to what I understand is the problem is that there should be no fields saved with empty values. What do you think about this?

[skjnldsv]

@xh3n1 when you click the new contact button, a new contact is created with the default REQUIRED fields according to the vcard rfc.
A basic contact MUST have one FN field set, but it cannot be empty, so we need it to be filled with something on creation or the server will refuse it.

That being said, if we create a temporary contact without any fields displayed BUT waiting to be synced as soon as the FN (name) field is filled, we could have something viable! :)

[xh3n1]

So when I'm creating a contact now the behavior is that it is added to the list of contacts with empty fields and with the name of New contact. The expected behavior is that the new contact is not added until the name is changed from New contact to something else and no fields are saved in it unless they have a non empty string in them as values. Am I correct @skjnldsv ?

[skjnldsv]

You're correct! 😉

[jancborchardt]

@jonatoni @xh3n1 is this now changed as @skjnldsv asked? Then we can review. :)

[xh3n1]

hey @skjnldsv can you review it now?

[skjnldsv]

Add some space between the = :)

[skjnldsv]

Why not use the condition directly in the if line 218?

[skjnldsv]

The contact will still have a valid fn set at this point. See line 202

if (_.isUndefined(newContact.fullName()) || newContact.fullName() === '') {
    newContact.fullName(t('contacts', 'New contact'));		 			
}

[skjnldsv]

Do not add unnecessary empty spaces :)

[skjnldsv]

Do not add unnecessary empty spaces :)

[skjnldsv]

Okay, I'll list the problem with the creation of a local contact only:

• contact_services.js:

  • The create function is used by the user input AND the import button, so we can't remove the xhr (XMLHttpRequest) request part from this function or we'll break the import.
  • The create function is returning a promise, used by ctrl.createContact in the newContactButton_controller.js, so we need to return something else (like the contact for example) and edit the controller to use it instead.
  • My guess will be to create a new function for the local contacts, something like createLocalContact() which don't save a thing and that is used by newContactButton_controller.js.
  • When opening a contact, the function getById is called and generate a xhr request to retrieve the full vcard. As we don't send it to the server on creation, the card won't be found. We need to create a function to load the contact from cache ONLY and not from the server. We need to create another function that use the same system that getById but for local contacts and without server requests.
  • We could probably add a value to the contact model that indicate if the contact is local or not. This way we can indicate contactDetails_controller.js to use either getById or our freshly created function (getLocalById?) instead. As soon as a contact is synced, we should set this value to false since it won't be a local contact anymore.

• newContactButton_controller.js

  • Don't use the then function since we're not returning a promise anymore and set the create function as a variable instead.

[skjnldsv]

To resume: diagram.jpg

Edit: my bad, wrong click! :(

[jancborchardt]

Seems to not work at the moment? Any updates or known issues? :)

[jancborchardt]

Also was thinking about showing the fields for IM, Social network and Notes by default too. Then we have a very nice 2-column layout: