Skip to content

Conversation

escopecz
Copy link
Member

@escopecz escopecz commented Mar 4, 2025

Q A
Bug fix? (use the a.b branch)
New feature/enhancement? (use the a.x branch) ✔️
Deprecations?
BC breaks? (use the c.x branch)
Automated tests included? ✔️
Related user documentation PR URL mautic/user-documentation#352
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #...

Description

This PR is adding a new command to delete custom fields by a command that can be run as a cron job the same as the existing commands that create and update custom fields.

This is the new command: bin/console mautic:custom-field:delete-column --id=[FIELD ID] --user=[USER ID]

If the FIELD ID is not provided, the command will look for fields that were soft-deleted (will be when create_custom_field_in_background is ON). If the USER ID is not provided, the user who modified the custom field the last when they were soft-deleting the field will be used.

There is no change for normal Mautic instances. This command is necessary only for Mautic instances with big leads database tables and busy traffic. Such instances should have the create_custom_field_in_background option set to true and use the background commands to manage custom fields as it modifies the leads table schema which is an expensive (read slow) operation and long running queries on that table while performing the schema change can time out and it could lead to data loss.

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Test that you can create and delete custom fields as you are used to. The changes in UI directly reflect the changes and the leads table is getting updated with those changes.
  3. Now, add this row to the config/local.php file: 'create_custom_field_in_background' => true, and clear the cache with rm -rf var/cache/*
  4. When you try to delete a custom fields via UI you can see the field is removed from the UI as before. But it still exists in the database and it is marked for deletion. We could call it soft-deleted.
  5. Run bin/console mautic:custom-field:delete-column. Even without the ID the command will find the soft-deleted field and remove it. Only after the command execution the field row in the lead_fleds table is removed and the column in the leads table is removed.

@escopecz escopecz added enhancement Any improvement to an existing feature or functionality custom-fields Anything related to custom fields unforking Used for PRs in the Acquia's unforking initiative labels Mar 4, 2025
Copy link

codecov bot commented Mar 4, 2025

Codecov Report

Attention: Patch coverage is 80.79470% with 29 lines in your changes missing coverage. Please review.

Project coverage is 64.70%. Comparing base (b25eee0) to head (2fc335c).
Report is 13 commits behind head on 7.x.

Files with missing lines Patch % Lines
...dBundle/Field/Command/DeleteCustomFieldCommand.php 51.51% 16 Missing ⚠️
app/bundles/LeadBundle/Form/Type/FieldType.php 25.00% 3 Missing ⚠️
app/bundles/LeadBundle/Field/BackgroundService.php 77.77% 2 Missing ⚠️
...Bundle/Field/Event/DeleteColumnBackgroundEvent.php 33.33% 2 Missing ⚠️
...ndles/LeadBundle/Field/Event/DeleteColumnEvent.php 60.00% 2 Missing ⚠️
...dle/Field/Notification/CustomFieldNotification.php 75.00% 2 Missing ⚠️
.../bundles/LeadBundle/Controller/FieldController.php 75.00% 1 Missing ⚠️
app/bundles/LeadBundle/Model/FieldModel.php 80.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                7.x   #14679      +/-   ##
============================================
+ Coverage     64.68%   64.70%   +0.02%     
- Complexity    34686    34736      +50     
============================================
  Files          2274     2279       +5     
  Lines        103607   103738     +131     
============================================
+ Hits          67015    67123     +108     
- Misses        36592    36615      +23     
Files with missing lines Coverage Δ
...s/LeadBundle/Controller/Api/FieldApiController.php 100.00% <100.00%> (ø)
app/bundles/LeadBundle/Entity/LeadField.php 97.55% <100.00%> (+0.94%) ⬆️
app/bundles/LeadBundle/Field/CustomFieldColumn.php 91.52% <100.00%> (+3.15%) ⬆️
.../Dispatcher/FieldColumnBackgroundJobDispatcher.php 100.00% <100.00%> (ø)
...dBundle/Field/Dispatcher/FieldColumnDispatcher.php 100.00% <100.00%> (ø)
...dBundle/Field/Dispatcher/FieldDeleteDispatcher.php 100.00% <100.00%> (ø)
app/bundles/LeadBundle/Field/LeadFieldDeleter.php 100.00% <100.00%> (ø)
.../bundles/LeadBundle/Controller/FieldController.php 35.68% <75.00%> (ø)
app/bundles/LeadBundle/Model/FieldModel.php 77.09% <80.00%> (-1.29%) ⬇️
app/bundles/LeadBundle/Field/BackgroundService.php 86.48% <77.77%> (-2.80%) ⬇️
... and 5 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@escopecz escopecz requested review from a team, rohitpavaskar and aarohiprasad and removed request for a team March 4, 2025 15:08
@escopecz escopecz added ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging labels Mar 4, 2025
@escopecz escopecz moved this to 🦸🏻 Needs 2 tests in Open Source Fridays Mar 4, 2025
@escopecz escopecz changed the base branch from 6.x to 7.x March 4, 2025 15:09
@escopecz escopecz changed the base branch from 7.x to 6.x March 4, 2025 15:16
Copy link
Contributor

@rohitpavaskar rohitpavaskar left a comment

Choose a reason for hiding this comment

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

Thanks @escopecz!. LGTM.

@escopecz escopecz requested a review from Copilot March 5, 2025 10:37
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.

kuzmany
kuzmany previously approved these changes Mar 6, 2025
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

I did basic tests with flag true/false.
Works as expected.
Code seems legit 👍

@escopecz escopecz changed the base branch from 6.x to 7.x March 7, 2025 12:59
@escopecz
Copy link
Member Author

escopecz commented Mar 7, 2025

This should be ready to merge once #14690 is merged.

@escopecz escopecz added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged blocked Something blocks this PR/issue (e.g. waiting for another PR to be merged) code-review-passed PRs which have passed code review user-testing-passed PRs which have been successfully tested by the required number of people. and removed ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging labels Mar 7, 2025
@escopecz escopecz removed the request for review from aarohiprasad March 7, 2025 13:07
@escopecz escopecz moved this from 🦸🏻 Needs 2 tests to 🎉 Ready to commit in Open Source Fridays Mar 7, 2025
@escopecz escopecz changed the base branch from 7.x to 6.x March 7, 2025 14:20
@escopecz escopecz dismissed kuzmany’s stale review March 7, 2025 14:20

The base branch was changed.

@escopecz escopecz changed the base branch from 6.x to 7.x March 7, 2025 15:50
@escopecz escopecz removed the blocked Something blocks this PR/issue (e.g. waiting for another PR to be merged) label Mar 7, 2025
@escopecz escopecz added this to the 7.0.0-alpha milestone Mar 7, 2025
@escopecz escopecz added the needs-documentation PR's that need documentation before they can be merged label Mar 7, 2025
@escopecz escopecz removed the needs-documentation PR's that need documentation before they can be merged label Mar 7, 2025
@escopecz escopecz merged commit 80b22f2 into mautic:7.x Mar 7, 2025
16 checks passed
@github-project-automation github-project-automation bot moved this from 🎉 Ready to commit to 🥳 Done in Open Source Fridays Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review-passed PRs which have passed code review custom-fields Anything related to custom fields enhancement Any improvement to an existing feature or functionality ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged unforking Used for PRs in the Acquia's unforking initiative user-testing-passed PRs which have been successfully tested by the required number of people.
Projects
Status: 🥳 Done
Development

Successfully merging this pull request may close these issues.

4 participants