After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 731175 - Modifying contact phone number get a timeout
Modifying contact phone number get a timeout
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: e-d-s backend
git master
Other Linux
: Normal critical
: Unset
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2014-06-03 20:27 UTC by Renato Araujo Oliveira Filho
Modified: 2014-06-20 13:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Path to fix the problem (2.30 KB, patch)
2014-06-10 15:33 UTC, Renato Araujo Oliveira Filho
needs-work Details | Review
New patch (2.14 KB, patch)
2014-06-19 01:23 UTC, Renato Araujo Oliveira Filho
none Details | Review
Update set_afd_equal function to compare AbstractFieldDetails values as string. (2.49 KB, patch)
2014-06-19 21:59 UTC, Renato Araujo Oliveira Filho
needs-work Details | Review
unit test (6.23 KB, patch)
2014-06-19 22:00 UTC, Renato Araujo Oliveira Filho
committed Details | Review
Fix all backends (6.90 KB, patch)
2014-06-20 13:06 UTC, Renato Araujo Oliveira Filho
committed Details | Review

Description Renato Araujo Oliveira Filho 2014-06-03 20:27:15 UTC
After update the contact phone number from 33333332 to 033333332 causes a timeout on the update function.

The contact get update on EDS but not in folks.
Comment 1 Renato Araujo Oliveira Filho 2014-06-03 20:32:11 UTC
After some investigation, I found the problem in esdf-persona.vala code:

inside of the function: private void _update_phones (bool create_if_not_exist, bool emit_notification = true)


on this code:

      if (!Folks.Internal.equal_sets<PhoneFieldDetails>  (new_phone_numbers,
              this._phone_numbers))
        {
          this._phone_numbers = new_phone_numbers;
          this._phone_numbers_ro = new_phone_numbers.read_only_view;
          if (emit_notification)
            {
              this.notify_property ("phone-numbers");
            }
        } 


In my point of view the problem is, after call the change function folks call the necessary function in EDS, which causes the change in the contact and notify folks about that(Edsf.PersonaStore::_contacts_changed_idle) this function try to update the contact information but folks uses a internal function to compare phone numbers and that functions returns that the phone number is equal (which is wrong), and because of that folks does not notify about property change, and now we have different phone numbers on folks and EDS.
Comment 2 Travis Reitter 2014-06-04 16:54:01 UTC
Check out folks/phone-details.vala. There's some code in there to normalize numbers and strip off extensions (eg, _drop_extension). It sounds like there's a bug where we accidentally strip off some leading numbers (which could make these two numbers compare as equal).
Comment 3 Renato Araujo Oliveira Filho 2014-06-08 22:33:47 UTC
(In reply to comment #2)
> Check out folks/phone-details.vala. There's some code in there to normalize
> numbers and strip off extensions (eg, _drop_extension). It sounds like there's
> a bug where we accidentally strip off some leading numbers (which could make
> these two numbers compare as equal).

I think the problem is that we should not use phone comparison functions on this case, I should be able to update the phone number for any value that I want. 
Phone number comparison is ok for match contacts but not to check if they change or not.

For example if I travel abroad and I want to update my contact phone from 754-3010 (local number) to (541) 754-3010 (with area code), this comparison will always block me to update and I will get a update error due the phone number comparison.

In my point of view we should use a plain string to check if they are equal or not inside of the update function.

What do you think?
Comment 4 Philip Withnall 2014-06-09 15:20:29 UTC
(In reply to comment #3)
> In my point of view we should use a plain string to check if they are equal or
> not inside of the update function.
> 
> What do you think?

As discussed on IRC, this sounds entirely reasonable to me.
Comment 5 Renato Araujo Oliveira Filho 2014-06-09 18:34:42 UTC
I need help to run the eds tests.

I tried with make check but it get stuck when trying to run EDS tests, all the others tests works nice.

I tried the instructions in the HACKING file and it get stuck too.
Comment 6 Erick Perez Castellanos 2014-06-10 15:28:13 UTC
Maybe, you should check, this sounds a ot like like bug 701990
Comment 7 Renato Araujo Oliveira Filho 2014-06-10 15:33:54 UTC
Created attachment 278211 [details] [review]
Path to fix the problem

Compare phone number as string during the field update.
Comment 8 Renato Araujo Oliveira Filho 2014-06-10 15:41:47 UTC
I am unable to write unit test for this due a bug on eds tests, check: https://bugzilla.gnome.org/show_bug.cgi?id=731464
Comment 9 Philip Withnall 2014-06-18 22:39:05 UTC
Review of attachment 278211 [details] [review]:

With bug #731464 fixed, some unit tests can be written. :-)

::: backends/eds/lib/edsf-persona.vala
@@ +1975,3 @@
+      foreach (PhoneFieldDetails a_elem in a)
+        {
+          PhoneFieldDetails found = null;

This should be nullable.

@@ +1991,3 @@
+            {
+              // avoid compare with the same element more than once
+              b.remove(found);

This is wrong. It modifies @b, which from the call site is @new_phone_numbers — so you will end up emptying @new_phone_numbers and thus storing an empty set.

You should not need to remove elements to avoid comparing against them twice. Was there a particular test failure which prompted this code?

@@ +2035,3 @@
+      // numbers and will remove the prefix, and that could cause a wrong result
+      // since the the phone number is stored as string
+      if (!_phone_set_is_equal (this._phone_numbers, new_phone_numbers))

s/_phone_set_is_equal/this._phone_set_is_equal/ to make it more explicit.
Comment 10 Renato Araujo Oliveira Filho 2014-06-19 01:23:20 UTC
Created attachment 278726 [details] [review]
New patch
Comment 11 Renato Araujo Oliveira Filho 2014-06-19 01:25:27 UTC
(In reply to comment #9)
> Review of attachment 278211 [details] [review]:
> 
> With bug #731464 fixed, some unit tests can be written. :-)
> 

The tests still crashing for me with: 
../../test-driver: line 107: 17439 Trace/breakpoint trap   "$@" > $log_file 2>&1
FAIL: persona-store-tests
../../test-driver: line 107: 17476 Trace/breakpoint trap   "$@" > $log_file 2>&1
FAIL: individual-retrieval
../../test-driver: line 107: 17515 Trace/breakpoint trap   "$@" > $log_file 2>&1
FAIL: phone-details
...

I tried a brand new image on my VM, and got the same results, I do not have any idea how to fix that. 

Are you able to run the tests? Which dist are you using?
Comment 12 Renato Araujo Oliveira Filho 2014-06-19 21:59:57 UTC
Created attachment 278808 [details] [review]
Update set_afd_equal function to compare AbstractFieldDetails values as string.

This is necessary to avoid problems with phone numbers that has a special values_equal function to try match phone numbers in different formats.
Comment 13 Renato Araujo Oliveira Filho 2014-06-19 22:00:26 UTC
Created attachment 278809 [details] [review]
unit test 

unit test for change the phone number prefix.
Comment 14 Philip Withnall 2014-06-20 09:21:46 UTC
Review of attachment 278809 [details] [review]:

Looks good to me. Have you verified that the test case fails without your patch to fix the problem applied?
Comment 15 Philip Withnall 2014-06-20 09:27:13 UTC
Review of attachment 278808 [details] [review]:

Please update NEWS and include a bug reference in the commit message.

::: folks/utils.vala
@@ +188,3 @@
   public static bool set_afd_equal (
+      Set<AbstractFieldDetails<string> > a,
+      Set<AbstractFieldDetails<string> > b)

You can’t change the definition of this method, since it’s used in other places in libfolks, such as for comparing AbstractFieldDetailsMRole>s in individual.vala.

Please create a new method in utils.vala:
    public static bool set_phones_afd_equal (Set<AbstractFieldDetails<string>> a,
                                             Set<AbstractFieldDetails<string>> b)
and use that. Then other backends can also use it for comparing sets of phone numbers.

In fact, could you please make the necessary changes in the other backends too? Thanks!
Comment 16 Renato Araujo Oliveira Filho 2014-06-20 13:06:50 UTC
Created attachment 278833 [details] [review]
Fix all backends
Comment 17 Philip Withnall 2014-06-20 13:17:21 UTC
Review of attachment 278833 [details] [review]:

++