GNOME Bugzilla – Bug 731175
Modifying contact phone number get a timeout
Last modified: 2014-06-20 13:23:57 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.
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.
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).
(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?
(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.
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.
Maybe, you should check, this sounds a ot like like bug 701990
Created attachment 278211 [details] [review] Path to fix the problem Compare phone number as string during the field update.
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
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.
Created attachment 278726 [details] [review] New patch
(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?
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.
Created attachment 278809 [details] [review] unit test unit test for change the phone number prefix.
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?
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!
Created attachment 278833 [details] [review] Fix all backends
Review of attachment 278833 [details] [review]: ++