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/required field does not display as required in the WP Mailchimp settings page #109

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

Conversation

MaxwellGarceau
Copy link
Collaborator

@MaxwellGarceau MaxwellGarceau commented Jan 3, 2025

Description of the Change

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

merge-field-required-label-does-not-appear-in-wp-admin.mov

Solution

Update strict equality comparison check to force the required field to have the same type. This fixes the bug and will cause merge fields to correctly display if they are required in the Mailchimp WP admin page.

Closes #102

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. Log into the test user Mailchimp account and set a sampling of the merge fields to required.
  2. Log into the WordPress site -> navigate to the Mailchimp settings page -> Click "Update List" to refresh the Mailchimp data.
  3. Navigate down to the merge field tags section. The required labels should reflect whether the field is or is not required in the Mailchimp account.

Changelog Entry

Fixed - Fixed bug causing merge fields in the Mailchimp WP admin page to falsely display as not required when in fact they were required

Credits

Props @MaxwellGarceau

Checklist:

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

@github-actions github-actions bot added this to the 1.7.0 milestone Jan 3, 2025
@MaxwellGarceau MaxwellGarceau marked this pull request as ready for review January 3, 2025 01:07
@MaxwellGarceau MaxwellGarceau requested a review from dkotter January 3, 2025 01:07
@github-actions github-actions bot added the needs:feedback This requires reporter feedback to better understand the request. label Jan 3, 2025
Copy link

github-actions bot commented Jan 3, 2025

@MaxwellGarceau thanks for the PR! Could you please fill out the PR template with description, changelog, and credits information so that we can properly review and merge this?

@github-actions github-actions bot added needs:code-review This requires code review. and removed needs:feedback This requires reporter feedback to better understand the request. labels Jan 3, 2025
@MaxwellGarceau
Copy link
Collaborator Author

@MaxwellGarceau thanks for the PR! Could you please fill out the PR template with description, changelog, and credits information so that we can properly review and merge this?

Done

@MaxwellGarceau MaxwellGarceau changed the title Add intval to required field to force same type comparison Bugfix: Required field does not display as required in the WP Mailchimp settings page Jan 3, 2025
@MaxwellGarceau MaxwellGarceau changed the title Bugfix: Required field does not display as required in the WP Mailchimp settings page Bug/required field does not display as required in the WP Mailchimp settings page Jan 3, 2025
@MaxwellGarceau MaxwellGarceau requested review from qasumitbagthariya and removed request for qasumitbagthariya January 3, 2025 21:50
@MaxwellGarceau
Copy link
Collaborator Author

Removing request for a review from @qasumitbagthariya so that @dkotter can be the person to advance tickets to the next stage (read your note on #97 (comment) just a little too late haha).

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
Development

Successfully merging this pull request may close these issues.

When a Mailchimp field is required in the Mailchimp account the merge field still displays "N" in the WP admin
2 participants