Fix: add distinct to donation forms list query to prevent duplicate results

[jonwaldstein]

Resolves GIVE-109

Description

This adds the SQL distinct statement to the donation forms list query to prevent duplicate results.

Affects

The donation forms list table results

Visuals

Before

After

Testing Instructions

• Create a few forms and view the donation forms list
• Make sure there are not duplicate results
• Try searching for a title or ID and make sure there are not duplicate results

Pre-review Checklist

• Acceptance criteria satisfied and marked in related issue
• Relevant @unreleased tags included in DocBlocks
• Includes unit tests
• Reviewed by the designer (if follows a design)
• Self Review of code and UX completed

[JasonTheAdams]

@jonwaldstein I'm curious what the underlying SQL looks like that is returning multiple forms. That usually alludes to something odd in the query, and adding DISTINCT compensates for that. This isn't necessarily true, but when the form is the top-most object, it's strange that it can return multiple rows per object.

This is worth checking as, if it is query strangeness, fixing the query can result in performance improvements.

[alaca]

👍

[alaca]

As @JasonTheAdams mentioned, something is odd here. I tried to reproduce the issue, but I wasn't able to.

[jonwaldstein]

@JasonTheAdams @alaca I did look at the SQL at first but nothing looked out of the ordinary so I figured distinct would be a quick win for people frustrated by this. I'll dig in a little farther and see if I can pin point if it's a query/sql issue.

[JasonTheAdams]

Thanks @jonwaldstein! Feel free to drop the resulting SQL here if you think more eyes would help.

[alaca]

Here is the generated SQL.

SELECT ID                                                  AS id,
       post_title                                          AS title,
       post_date                                           AS createdAt,
       post_modified                                       AS updatedAt,
       post_status                                         AS status,
       give_formmeta_attach_meta_formEarnings.meta_value   AS formEarnings,
       give_formmeta_attach_meta_formSales.meta_value      AS formSales,
       give_formmeta_attach_meta_donationLevels.meta_value AS donationLevels,
       give_formmeta_attach_meta_setPrice.meta_value       AS setPrice,
       give_formmeta_attach_meta_priceOption.meta_value    AS priceOption,
       give_formmeta_attach_meta_goalOption.meta_value     AS goalOption
FROM wp_posts
         LEFT JOIN wp_give_formmeta give_formmeta_attach_meta_formEarnings
                   ON ID = give_formmeta_attach_meta_formEarnings.form_id AND
                      give_formmeta_attach_meta_formEarnings.meta_key = '_give_form_earnings'
         LEFT JOIN wp_give_formmeta give_formmeta_attach_meta_formSales
                   ON ID = give_formmeta_attach_meta_formSales.form_id AND
                      give_formmeta_attach_meta_formSales.meta_key = '_give_form_sales'
         LEFT JOIN wp_give_formmeta give_formmeta_attach_meta_donationLevels
                   ON ID = give_formmeta_attach_meta_donationLevels.form_id AND
                      give_formmeta_attach_meta_donationLevels.meta_key = '_give_donation_levels'
         LEFT JOIN wp_give_formmeta give_formmeta_attach_meta_setPrice
                   ON ID = give_formmeta_attach_meta_setPrice.form_id AND
                      give_formmeta_attach_meta_setPrice.meta_key = '_give_set_price'
         LEFT JOIN wp_give_formmeta give_formmeta_attach_meta_priceOption
                   ON ID = give_formmeta_attach_meta_priceOption.form_id AND
                      give_formmeta_attach_meta_priceOption.meta_key = '_give_price_option'
         LEFT JOIN wp_give_formmeta give_formmeta_attach_meta_goalOption
                   ON ID = give_formmeta_attach_meta_goalOption.form_id AND
                      give_formmeta_attach_meta_goalOption.meta_key = '_give_goal_option'
WHERE post_type = 'give_forms'
  AND post_status IN ('publish', 'draft', 'pending', 'private', 'upgraded')

[jonwaldstein]

Okay I found the problem, duplicate meta keys.

The query & underlying SQL is fine. It's that there are some form meta keys that are duplicated (will have to figure out why)

[alaca]

Hmmm, one of the reasons might be that there are more than one meta keys with the same name.

[alaca]

Yep, that's it.

[JasonTheAdams]

I wonder if in some v2-v3 compatibility places, we're using add_post_meta when we should be using update_post_meta?

[alaca]

There isn't much that we can do for existing users, except to use DISTINCT in query. There is one more option but it's more "costly" and requires some additional work than using DISTINCT.

We should find where things went wrong and fix them, but the additional meta is already in the database. We can clear that up by adding a migration, but is it worth it?

[jonwaldstein]

@alaca if we can figure out where the problem originates, remove it and add a migration - that would be my preference.

However, I realize that could take some time that we might not currently have available - so I think simply using distinct for now would still be a good solution for the interim (considering 34 people have already upvoted this issue on canny).

@JasonTheAdams to your point, we are compensating by using distinct, but it seems to be warranted until we can figure out the underlying problem of duplicate meta keys.

[JasonTheAdams]

I happened upon this video and it helped me understand our predicament: https://youtu.be/5hsl47I3svw?si=c7hP-jglwkD7YQuh

The issue is that our LEFT JOIN meta lines will create a duplicate line if there's more than one meta line. That means this is more than just an issue with our list table, but could happen any time we query a donation and join the meta. We may consider putting a LIMIT 1 on the attachMeta queries. 🤔

[alaca]

I'm not sure we can just add LIMIT to the JOIN query, at least not in the way we are writing queries now. We have to use JOIN because we need data from the meta table, but we might try a slightly different approach - JOIN with subquery. Then we can limit the result of the subquery.

[JasonTheAdams]

@alaca Yeah, I had a feeling a LIMIT can't be applied to a join in this way. I think it could be worth experimenting with a sub-query join instead of a table join. It's definitely a gnarly bug of how attachMeta currently works, which can easily throw us off in other places. As it is, if we apply DISTINCT here we're just compensating for a (now) known query builder bug. I'd rather fix it at the source.

[jonwaldstein]

@JasonTheAdams i'm on board with fixing at the source, however that will affect every query in existence using the QB and will require a lot more testing. I think we should release this as a hot fix for now (considering how many customers this is affecting) - making sure to circle back around when the source is resolved.

[alaca]

I agree with Jon, we should probably release this as a hot fix.

Also, I saw the task for replacing the Give QB with the QB package from Stellar, maybe it would be a good idea to resolve this issue there first. I also have ideas on how to improve the QB, mainly insert, update, and delete methods.

[JasonTheAdams]

Since adding DISTINCT won't adversely affect the fix we're discussing, I'm onboard with releasing this as a hot fix tomorrow — not today, as it's not breaking any donations, and it's not worth introducing risk. Also, since 3.1.2 is effectively merged, there's no way to do a hot fix without releasing all of that as well.

@alaca I'm ok with including this as part of the Stellar DB work, but that puts it in a much higher priority for me. We now have a known bug that affects any model attaching meta. Mind you, I don't think duplicate meta happens too often, and this only affects QueryBuilder::getAll() (since QueryBuilder::get() only grabs the first row). Still, not something we want to ignore as it's difficult to predict where it will occur.

[jonwaldstein]

@JasonTheAdams sounds good, i'll get this into QA and create a new issue to tackle the QB part.

[rickalday]

I tested this on a customer's staging site that was experiencing the problem and it works.