Created attachment 273361 [details] [review]
Fixed FOLKS_DISABLE_LINKING.

Created attachment 273362 [details] [review]
unit test for FOLKS_DISABLE_LINKING.

Review of attachment 273362 [details] [review]:

::: tests/dummy/Makefile.am
@@ +45,3 @@
+disable_link_SOURCES = \
+	disable-link.vala \
+	$(NULL)

I think you forgot to `git add disable-link.vala`. :-)

::: tests/lib/dummy/test-case.vala
@@ +136,3 @@
+      var persona_stores = new HashSet<PersonaStore> ();
+      persona_stores.add (this.dummy_persona_store);
+      this.dummy_backend.unregister_persona_stores (persona_stores);

This is a separate change which should be in a separate patch. (But looks like a good change, thanks.)

Review of attachment 273361 [details] [review]:

It’s not clear to me from this patch where the problem is in the current code. Could you please add to your commit message a more detailed explanation of where FOLKS_DISABLE_LINKING is failing? I don’t want any changes committed to the aggregation code without thorough justification. :-)

::: folks/individual-aggregator.vala
@@ +1249,3 @@
 
+          if (this._linking_enabled == true &&
+              persona.store.trust_level == PersonaStoreTrust.FULL)

I don’t think any of the three above changes are necessary, because the “if (candidate_inds.size > 0 && this._linking_enabled == true)” check lower down includes all three of the above checks.

@@ +1406,3 @@
+      /* Ignore it if the link is disabled */
+      if (this._linking_enabled == true)
+        {

Could you rearrange it to the following please, to avoid excess indentation and whitespace churn?

if (this._linking_enabled == false)
  {
    return;
  }

/* Existing code… */

(In reply to comment #4)
> Review of attachment 273361 [details] [review]:
> 
> It’s not clear to me from this patch where the problem is in the current code.
> Could you please add to your commit message a more detailed explanation of
> where FOLKS_DISABLE_LINKING is failing? I don’t want any changes committed to
> the aggregation code without thorough justification. :-)
> 

I will need you help here :D,  is not clear for me why this is failing, what I know is that inside of the function " private void _add_personas (" you populate the candidate_inds with the possible candidates, and then in the end of the function you go thought all candidates doing some extra changes.

          ...
          foreach (var i in candidate_inds)
            {
              /* Remove the old individuals from the link map. */
              this._remove_individual_from_link_map (i);
          ...

And this cause the individuals to get linked. What I did was avoid the candidate_inds get populate, and looks like this get fixed.

Created attachment 273440 [details] [review]
Fixed FOLKS_DISABLE_LINKING.

Created attachment 273441 [details] [review]
unit test for FOLKS_DISABLE_LINKING

Review of attachment 273440 [details] [review]:

(In reply to comment #5)
> (In reply to comment #4)
> > Review of attachment 273361 [details] [review] [details]:
> > 
> > It’s not clear to me from this patch where the problem is in the current code.
> > Could you please add to your commit message a more detailed explanation of
> > where FOLKS_DISABLE_LINKING is failing? I don’t want any changes committed to
> > the aggregation code without thorough justification. :-)
> > 
> 
> I will need you help here :D,  is not clear for me why this is failing, what I
> know is that inside of the function " private void _add_personas (" you
> populate the candidate_inds with the possible candidates, and then in the end
> of the function you go thought all candidates doing some extra changes.
> 
>           ...
>           foreach (var i in candidate_inds)
>             {
>               /* Remove the old individuals from the link map. */
>               this._remove_individual_from_link_map (i);
>           ...
> 
> And this cause the individuals to get linked. What I did was avoid the
> candidate_inds get populate, and looks like this get fixed.

That make sense, thanks for the explanation. I didn’t notice that bit of code in my review.

In that case, I’d suggest you make an additional change to add the following line just before the “if (candidate_inds.size > 0 && this._linking_enabled == true)”:
    assert (this._linking_enabled == true || candidate_inds.size == 0);
Just to ensure this doesn’t get broken in future. Also, can you please make sure you explain your changes in the git commit message? Thanks.

Review of attachment 273441 [details] [review]:

::: tests/dummy/Makefile.am
@@ +44,3 @@
 
+disable_link_SOURCES = \
+	disable-link.vala \

This filename doesn’t match the file you’ve added in git (‘disable-autolink.vala’). Have you actually compiled and tested this? I would expect the new test to fail before applying your patch which fixes FOLKS_DISABLE_LINKING, and to pass after applying it.

::: tests/dummy/disable-autolink.vala
@@ +52,3 @@
+          this.test_linkable_properties_aggregate_after_change);
+      //this.add_test ("Test manually link personas even with auto-link disabled",
+      //   this.test_manual_link_with_autolink_disable);

I don’t understand how this test is meant to work. Where does it set FOLKS_DISABLE_LINKING?

I would expect the test to set FOLKS_DISABLE_LINKING, then to add two personas which would normally get linked together, and then to verify that they’re _not_ linked together.

@@ +231,3 @@
+                  this._aggregator.link_personas(personas);
+                  this._ind_linked = true;
+                  */

Please remove code instead of commenting it out.

@@ +249,3 @@
+              var i = iter.get ();
+              GLib.debug ("Persona size: %d\n", i.personas.size);
+              //assert (i.personas.size == 2);

Why is this assertion commented out?

::: tests/lib/dummy/test-case.vala
@@ +136,3 @@
+      var persona_stores = new HashSet<PersonaStore> ();
+      persona_stores.add (this.dummy_persona_store);
+      this.dummy_backend.unregister_persona_stores (persona_stores);

…still needs splitting into a separate patch.

Created attachment 273528 [details] [review]
Fixed FOLKS_DISABLE_LINKING.

Created attachment 273529 [details] [review]
Fixed dummy test case

Created attachment 273530 [details] [review]
unit test for FOLKS_DISABLE_LINKING

(In reply to comment #9)
> Review of attachment 273441 [details] [review]:
> 
> ::: tests/dummy/Makefile.am
> @@ +44,3 @@
> 
> +disable_link_SOURCES = \
> +    disable-link.vala \
> 
> This filename doesn’t match the file you’ve added in git
> (‘disable-autolink.vala’). Have you actually compiled and tested this? I would
> expect the new test to fail before applying your patch which fixes
> FOLKS_DISABLE_LINKING, and to pass after applying it.
> 
> ::: tests/dummy/disable-autolink.vala
> @@ +52,3 @@
> +          this.test_linkable_properties_aggregate_after_change);
> +      //this.add_test ("Test manually link personas even with auto-link
> disabled",
> +      //   this.test_manual_link_with_autolink_disable);
> 
> I don’t understand how this test is meant to work. Where does it set
> FOLKS_DISABLE_LINKING?
> 
> I would expect the test to set FOLKS_DISABLE_LINKING, then to add two personas
> which would normally get linked together, and then to verify that they’re _not_
> linked together.
> 
> @@ +231,3 @@
> +                  this._aggregator.link_personas(personas);
> +                  this._ind_linked = true;
> +                  */
> 
> Please remove code instead of commenting it out.
> 
> @@ +249,3 @@
> +              var i = iter.get ();
> +              GLib.debug ("Persona size: %d\n", i.personas.size);
> +              //assert (i.personas.size == 2);
> 
> Why is this assertion commented out?
> 
> ::: tests/lib/dummy/test-case.vala
> @@ +136,3 @@
> +      var persona_stores = new HashSet<PersonaStore> ();
> +      persona_stores.add (this.dummy_persona_store);
> +      this.dummy_backend.unregister_persona_stores (persona_stores);
> 
> …still needs splitting into a separate patch.

Very sorry, this was my mistake I added the wrong test file. I fixed that on the new patch.

Review of attachment 273528 [details] [review]:

Looks good to me.

Review of attachment 273529 [details] [review]:

Also looks good.

Review of attachment 273530 [details] [review]:

This should be rewritten as a test in the tests/folks directory, since it's testing the core of folks. Tests in the tests/dummy directory are for testing features of the dummy backend only. You could rebase the code on the existing tests/folks/standalone-individuals.vala file, since that uses the dummy backend as part of the fixture for its tests.

I pushed the first two patches with tweaks to their commit messages. As soon as you update the unit test patch, that can be reviewed and committed too. Thanks for your work on this!

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/folks/issues/93.