Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create campaign tables #7519

Merged
merged 11 commits into from
Aug 27, 2024
Merged

Conversation

alaca
Copy link
Member

@alaca alaca commented Aug 26, 2024

Description

This PR is reusing the give_campaigns table created by P2P. Also, it adds a new column to the give_campaigns table - campaign_type. The give_core_campaign_forms table is a lookup table that exists solely to avoid possible performance issues when using the post meta table.

Pre-review Checklist

  • Relevant @unreleased tags included in DocBlocks
  • Self Review of code and UX completed

@JasonTheAdams
Copy link
Contributor

@alaca The intent is to re-use the existing P2P give_campaigns table, right? Ultimately, we want the two merged into a single table with a type that differentiates the various campaign types.

@alaca
Copy link
Member Author

alaca commented Aug 26, 2024

@JasonTheAdams we talked about it and the plan was to create a new table, at least I understood it like that. The campaigns table created from P2P seems too oriented on P2P (it has columns like form_id, campaign_url, etc. ) which is something we don't need here. But yeah, we can definitely make use of the give_campaigns table.

@JasonTheAdams
Copy link
Contributor

@alaca I see, however I disagree with that conclusion. Ultimately, we want to be able to query all the campaigns at once. It sounds more like we need to offload some of the columns from the existing table to another P2P-specific table. It was intended to be generic when we built it, seems like some specific columns snuck their way in when we built it 3 years ago. 🤷‍♂️

@kjohnson
Copy link
Member

My understanding was that we would migrate the table from P2P to core.

@JasonTheAdams
Copy link
Contributor

JasonTheAdams commented Aug 26, 2024

@kjohnson Yeah, so we'll want to make two migrations:

  1. A conditional one in core that creates the table if it doesn't already exist
  2. A P2P migration to move some of the P2P-specific columns into the give_p2P_campaigns table, and moves the column data over

@alaca
Copy link
Member Author

alaca commented Aug 26, 2024

@JasonTheAdams @kjohnson okay, then we will use the existing give_campaigns table. We will just have to add some conditional stuff.

@JasonTheAdams
Copy link
Contributor

Thanks @alaca! You're the best! 🙌

@JasonTheAdams
Copy link
Contributor

As a note, @alaca, this is a good time to assess the existing columns — names, types, etc. — and make sure they're what we want. Since we're making migrations to move and adjust things now, best to give it some thought and not have to make more later. I think it's worth some discussion.

@alaca
Copy link
Member Author

alaca commented Aug 26, 2024

@JasonTheAdams @kjohnson I updated the table name and the columns list to be the same as the one created from P2P.
And the awesome thing here is that we don't have to add any checks because we use dbDelta. dbDelta will check if that table exists, and if not, it will create one. Also, it will check the columns and update the table if needed. So the only extra column I have to add here is campaign_type.

@kjohnson kjohnson merged commit 3741176 into epic/campaigns Aug 27, 2024
20 checks passed
@kjohnson kjohnson deleted the feature/campaign-table-GIVE-1137 branch August 27, 2024 16:23
Copy link
Member

@kjohnson kjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved (forgot to click this button before merging into the epic).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants