Prevent promotion rule to show user multiple times

[vzqzac]

Description

The ransack query used to fetch users for the promotion rule dropdown checked for attributes that may return repeated results. This adds the distinct flag in the result's call to prevent repeated records to be shown.
Fixes #3183

This affects the API's ResourceController, so the scope of the update may be wide, although the change will likely fix un-encountered issues (IMO we shouldn't be returning repeated records independently of the API call, but open to discuss)

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• [ ] I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)

[vzqzac]

I'm not totally sure if the approach I followed for the spec is the best one, I'd happily change it if a better approach can be accomplished

[ericsaupe]

LGTM!

[tvdeyen]

LGTM. Thanks 🌮

[kennyadsl]

We discussed this in the core meeting and we think that this is a bit too extreme as solution. 😬

If we are having multiple results there is the possibility that there are wrong table joins and it would be great to understand where is the problem and fix just that. What about going a little bit deeper and try to understand where the duplicate come from?

[vzqzac]

@kennyadsl it is indeed! The actual source of the problem relies in the user picker javascript:

solidus/backend/app/assets/javascripts/spree/backend/user_picker.js

Lines 28 to 33 in a3cc823

	q: {
	m: 'or',
	email_start: term,
	addresses_firstname_start: term,
	addresses_lastname_start: term
	}

Whit that Ransack query, the resulting pg query we obtain when giving an input "a" is something like:

[2] pry(#<Spree::Api::UsersController>)> collection_scope.ransack(params[:q]).result
  Spree::User Load (0.8ms)  SELECT "spree_users".* FROM "spree_users" LEFT OUTER JOIN
 "spree_user_addresses" ON "spree_user_addresses"."user_id" = "spree_users"."id" AND
 "spree_user_addresses"."archived" = $1 LEFT OUTER JOIN "spree_addresses" ON
 "spree_addresses"."id" = "spree_user_addresses"."address_id" WHERE "spree_users"."deleted_at" IS
 NULL AND (("spree_users"."email" ILIKE 'a%' OR "spree_addresses"."firstname" ILIKE 'a%') OR
 "spree_addresses"."lastname" ILIKE 'a%')  [["archived", false]]

The easiest workaround I found was to just set the distinct true parameter, but I totally understand how extreme this is, given the controller where this happens. This could be done in a view too, but unfortunately, the call is done via AJAX, we can update that to sanitize though, but doesn't seem like a proper solution in my opinion.

Do you have any suggestion? I'm happy to discuss this and update the code with whatever we come up to 😃

[kennyadsl]

@vzqzac thanks for the detailed report!

I was investigating a little bit further and I found a few things:

• From what I can read in the Ransack REAMDE, this is a known issue, and to fix it distinct: true alone could not work with all the scenarios. It looks like sometime you also have to add some explicit includes/joins to the ransack call. If this is true we may break some extensions or stores that are using Ransack relying on ResourceController, right?
• This piece of code in the backend users controller. It looks like we are doing something very similar there with a custom query. I'm not sure it's better and honestly, I don't even know if we are using it but it may be worth investigating why that piece of code exists and if we need something similar in the users API controller as well.

Let me know your thoughts. 🙂

Dismissing review after a past core team meeting discussion

[vzqzac]

I'm closing this since I haven't worked on it from a long time now, I'll try to open a new pull request based on @kennyadsl suggestions