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

Bug/merge fields not marked as "Visible?" in the user's Mailchimp account will still display as includable in the WP Mailchimp settings page #110

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

MaxwellGarceau
Copy link
Collaborator

Description of the Change

In order to avoid duplicating information, please see #105 for a description of the original bug.

non-visible-merge-fields-can-still-be-set-in-the-mailchimp-wp-admin.mov

Solution

Add another conditional check to display the include checkboxes only when the merge field is public.

Closes #105

How to test the Change

This video in the "Description of the Change" demonstrating the original bug can be used as a walkthrough. However, instead of the bug you should see the fix.

  1. Uncheck the "Visible?" column in a sampling of merge fields in the test user Mailchimp account
  2. On the Mailchimp WP settings page click "Update List" to refresh the data from Mailchimp
  3. In the "Merge Fields Included" section, you should only see checkboxes for the merge fields that have the "Visible?" column checked in the Mailchimp account
  4. (optional) Navigate to the front end to ensure that the non visible merge fields do not display.

Changelog Entry

Fixed - Fixed bug allowing a user to select merge fields that were not selected as visible in the Mailchimp account

Credits

Props @MaxwellGarceau

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly. No documentation to update
  • I have added tests to cover my change. Small fix, no test needed
  • All new and existing tests pass.

@github-actions github-actions bot added this to the 1.7.0 milestone Jan 3, 2025
@MaxwellGarceau MaxwellGarceau changed the title Add public as requirement to show include box on merge field Bugfix: Merge fields not marked as "Visible?" in the user's Mailchimp account will still display as includable in the WP Mailchimp settings page Jan 3, 2025
@MaxwellGarceau MaxwellGarceau marked this pull request as ready for review January 3, 2025 01:50
@MaxwellGarceau MaxwellGarceau requested a review from dkotter January 3, 2025 01:50
@github-actions github-actions bot added the needs:code-review This requires code review. label Jan 3, 2025
@MaxwellGarceau MaxwellGarceau changed the title Bugfix: Merge fields not marked as "Visible?" in the user's Mailchimp account will still display as includable in the WP Mailchimp settings page Bug/Merge fields not marked as "Visible?" in the user's Mailchimp account will still display as includable in the WP Mailchimp settings page Jan 3, 2025
@MaxwellGarceau MaxwellGarceau changed the title Bug/Merge fields not marked as "Visible?" in the user's Mailchimp account will still display as includable in the WP Mailchimp settings page Bug/merge fields not marked as "Visible?" in the user's Mailchimp account will still display as includable in the WP Mailchimp settings page Jan 3, 2025
@qasumitbagthariya
Copy link
Collaborator

QA Update ✅


I have verified this PR in the bug/non-visible-merge-fields-will-display-in-settings-menu-as-options-in-the-include-column branch which has been fixed and is functioning as intended.

I tested the following on this branch:

  1. In the "Merge Fields Included" section, only see checkboxes for the merge fields that have the "Visible?" column checked in the Mailchimp account
  2. Navigate to the front end to ensure that the non visible merge fields do not display. ✅
Screen.Recording.2025-01-18.at.3.29.05.PM.mov

Testing Environment

  • WordPress: 6.7.1
  • Theme: Twenty Twenty-Four 1.3
  • WooCommerce - 9.5.1
  • PHP: 8.0.30
  • Web Server: Nginx 1.20.2
  • Browser: Chrome
  • OS: macOS 15.2
  • Branch: bug/non-visible-merge-fields-will-display-in-settings-menu-as-options-in-the-include-column

Steps to Test- As mentioned in the PR description.
Test Results - It is working as expected.
Functional Demo / Screencast -
Special Notes - Ready for code review (Woo)
Testing Document status:
Cases related to this Issue/PR are added to the Critical Flow Wiki pages:

  • Yes
  • Not Required/Applicable for this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
3 participants