Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue that prevented new contacts from being saved #1416

Merged
merged 1 commit into from
Jan 18, 2020

Conversation

hanzi
Copy link
Member

@hanzi hanzi commented Jan 17, 2020

Whenever I would compile the JavaScript sources myself, I would run into an issue when creating a new contact:

  1. I would create a new contact and fill out some of its fields.
  2. The contact wouldn't save, the exclamation mark icon would just stay. If I reload the page at this point, the new contact is gone.
  3. If I then selected another contact and returned to the new one, all my previous changes were gone. But if I filled out some fields now, the contact finally got saved.

I can't reproduce this bug with the released version -- though it would still happen if I compiled the JavaScript from the v3.1.7, v3.1.6 or v3.1.5 tags myself. So I'm not sure whether this might be an issue with some dependency or I'm just doing something wrong.

Anyway, the issue as far as I could tell is that the contact watcher of the ContactDetails component fires twice when a new contact is added: once with the new contact and once with the old one. This eventually leads to localContact being overwritten with the old one, so that all save actions actually just save the old contact.

This PR fixes this behaviour by adding a check whether the new contact (inside the watcher) is actually different from the old one. I don't really like this fix because it only attacks the symptom and I still don't have any idea why the watcher is fired twice to begin with.

But at least this way the app is usable again for me.

Before, when creating a new contact the previously
selected contact would still be the 'localContact'.

That happened because the 'contact' watcher would be
called twice (once with the new, once with the old
contact).

Adding a check whether the selected contact has actually
changed remedies that.

Signed-off-by: Christian Kraus <hanzi@hanzi.cc>
@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #1416 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1416   +/-   ##
=======================================
  Coverage   62.31%   62.31%           
=======================================
  Files           4        4           
  Lines          69       69           
=======================================
  Hits           43       43           
  Misses         26       26

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 a38a728...8fadc6a. Read the comment docs.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, this doesn't hurt :)

@skjnldsv skjnldsv merged commit 6b995f4 into nextcloud:master Jan 18, 2020
@skjnldsv skjnldsv added 3. to review Waiting for reviews bug Something isn't working labels Jan 18, 2020
@skjnldsv skjnldsv added this to the next milestone Jan 18, 2020
@hanzi hanzi deleted the fix-new-contact branch January 18, 2020 08:39
@DBJRdev
Copy link

DBJRdev commented Jan 30, 2020

We encounter the same issue as hanzi wrote, but for us it only began when updating to Contacts 3.1.8 from the GUI within Nextcloud 17.0.2 - no compiling or building anything ourselves. It actually appeared in three of our deployments, all managed servers and one shared hosting at the provider Hetzner.

@hanzi
Copy link
Member Author

hanzi commented Jan 31, 2020

@DBJRdev That's odd -- this fix is already included in 3.1.8 and I don't have this issue anymore with Contacts 3.1.8 on Nextcloud 17.0.3.

Could you open a new issue with some more details (including the browser log)?

@DBJRdev
Copy link

DBJRdev commented Jan 31, 2020

@hanzi Did as you requested: #1452

@skjnldsv
Copy link
Member

Hey!
So this is actually the issue that created #1453

Because on the first save to server, we do not update the real full contact anymore.

The selectContact method also include a proper update of the localContact
All the changes are queued when editing a contact to avoid conflict. The first contact sent init the contact.dav property which is the dav library method object. Because we do not update the contact after the first sync anymore, the localContact used never have the ðav object, and therefore trigger a new vcard creation every time.

Fix incoming

@skjnldsv skjnldsv mentioned this pull request Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants