fix: custom taxonomies are not selectable in post form #1577
Conversation
WalkthroughThis update introduces dynamic taxonomy option loading and selection preservation in form builder multi-select fields, enhances category initialization on page load, adds new yellow-themed CSS utility classes, and displays an admin warning for plain permalinks. Changes span JavaScript, Vue, PHP, HTML templates, and CSS files. Changes
Assessment against linked issues
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
admin/form-builder/assets/js/components/field-multiselect/index.js (2)
27-61: Good implementation of dynamic options with room for improvement.The computed property correctly implements dynamic taxonomy option loading for exclude fields. However, consider these improvements for better code quality and robustness:
- if (this.option_field.name === 'exclude' && - this.editing_form_field && - this.editing_form_field.input_type === 'taxonomy' && - this.editing_form_field.name) { + if (this.option_field.name === 'exclude' && + this.editing_form_field?.input_type === 'taxonomy' && + this.editing_form_field?.name) { var taxonomy_name = this.editing_form_field.name; - if (wpuf_form_builder && wpuf_form_builder.wp_post_types) { + if (wpuf_form_builder?.wp_post_types) { for (var post_type in wpuf_form_builder.wp_post_types) { var taxonomies = wpuf_form_builder.wp_post_types[post_type]; - if (taxonomies && taxonomies.hasOwnProperty(taxonomy_name)) { + if (taxonomies && Object.hasOwn(taxonomies, taxonomy_name)) { var tax_field = taxonomies[taxonomy_name]; - if (tax_field && tax_field.terms && tax_field.terms.length > 0) { + if (tax_field?.terms?.length > 0) { var options = {}; tax_field.terms.forEach(function(term) { - if (term && term.term_id && term.name) { + if (term?.term_id && term?.name) { options[term.term_id] = term.name; } }); return options; } } } } }These changes use optional chaining for safer property access and
Object.hasOwn()instead ofhasOwnProperty()for better security.🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 42-42: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
81-89: Proper selectize lifecycle management with minor improvement suggestion.The method correctly destroys existing selectize instances before creating new ones, preventing memory leaks and stale bindings.
- var $select = $(this.$el).find('.term-list-selector'); - if ($select[0] && $select[0].selectize) { - $select[0].selectize.destroy(); - } + var $select = $(this.$el).find('.term-list-selector'); + if ($select[0]?.selectize) { + $select[0].selectize.destroy(); + }Using optional chaining makes the code more concise and safe.
🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
assets/js/wpuf-form-builder-components.js (2)
444-480: Implement dynamic taxonomy options for better taxonomy field support.The implementation of the
dynamic_optionscomputed property successfully addresses the core issue of making custom taxonomies selectable. The logic correctly identifies taxonomy fields and generates options from the global taxonomy data.However, consider applying these improvements based on static analysis suggestions:
// Dynamic options for taxonomy fields dynamic_options: function () { // Check if this is a Selection Terms field for a taxonomy if (this.option_field.name === 'exclude' && this.editing_form_field && this.editing_form_field.input_type === 'taxonomy' && this.editing_form_field.name) { var taxonomy_name = this.editing_form_field.name; // Look for terms in the wp_post_types data - if (wpuf_form_builder && wpuf_form_builder.wp_post_types) { + if (wpuf_form_builder?.wp_post_types) { for (var post_type in wpuf_form_builder.wp_post_types) { var taxonomies = wpuf_form_builder.wp_post_types[post_type]; - if (taxonomies && taxonomies.hasOwnProperty(taxonomy_name)) { + if (taxonomies && Object.hasOwn(taxonomies, taxonomy_name)) { var tax_field = taxonomies[taxonomy_name]; - if (tax_field && tax_field.terms && tax_field.terms.length > 0) { + if (tax_field?.terms?.length > 0) { var options = {}; tax_field.terms.forEach(function(term) { - if (term && term.term_id && term.name) { + if (term?.term_id && term?.name) { options[term.term_id] = term.name; } }); return options; } } } } } // Return original options if not a taxonomy field or no dynamic options found return this.option_field.options || {}; }🧰 Tools
🪛 Biome (1.9.4)
[error] 457-457: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 461-461: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 461-461: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 464-464: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 467-467: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
496-510: Improved selectize lifecycle management prevents initialization conflicts.The enhanced
bind_selectizemethod correctly destroys existing instances before creating new ones, which prevents multiple initialization issues and memory leaks.Consider adding optional chaining for better safety:
bind_selectize: function () { var self = this; // Destroy existing selectize if it exists var $select = $(this.$el).find('.term-list-selector'); - if ($select[0] && $select[0].selectize) { + if ($select[0]?.selectize) { $select[0].selectize.destroy(); } $select.selectize({}).on('change', function () { self.value = $( this ).val(); }); },🧰 Tools
🪛 Biome (1.9.4)
[error] 502-502: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
admin/form-builder/assets/js/form-builder.js (3)
1113-1126: LGTM! Good initialization pattern with minor optimization opportunity.The initialization approach is solid - loading categories on page load and binding change events. The code properly checks for element existence before proceeding.
Consider adding a small optimization to prevent unnecessary calls:
// Bind change event $postTypeSelect.on('change', function() { + // Only proceed if the value actually changed + if (this.value !== this.previousValue) { populate_default_categories(this, false); + this.previousValue = this.value; + } });
1135-1137: Add validation for form_id.The form_id retrieval is good, but consider adding validation to ensure it's a valid numeric value.
// Get form ID from the form -var form_id = $('input[name="wpuf_form_id"]').val() || 0; +var form_id = parseInt($('input[name="wpuf_form_id"]').val()) || 0; +if (form_id < 0) { + form_id = 0; +}
1170-1172: Enhanced error handling with descriptive logging.Good improvement over the previous error handling. The console.log provides helpful debugging information.
Consider using
console.errorfor better categorization:-console.log('Error loading default categories:', error); +console.error('Error loading default categories:', error);assets/css/admin/form-builder.css (3)
3691-3699: Prefer CSS variables over hardcoded colors for yellow border utilities
The new.wpuf-border-yellow-200and.wpuf-border-yellow-300classes follow the existing naming and opacity patterns. For easier theme management and future palette updates, consider referencing a CSS custom property (for example,var(--wpuf-warning-yellow-200)) instead of embedding the rawrgb(254 240 138)andrgb(253 224 71)values directly.
5380-5383: Document hover-specific warning background utility
The.hover\:wpuf-bg-yellow-100:hoverutility correctly applies a pale yellow warning background on hover. To improve discoverability, please add this new class to your team’s CSS utility documentation alongside other background utilities (e.g.,.wpuf-bg-yellow-100).
5579-5582: Leverage a CSS variable for the warning focus ring color
You’ve added.focus\:wpuf-ring-yellow-500:focusand.focus\:wpuf-ring-offset-2:focusto highlight warning states. For consistency and maintainability, consider referencing a CSS custom property (e.g.,var(--wpuf-warning-ring)) for the ring color rather than hardcodingrgb(234 179 8).Also applies to: 5584-5586
assets/js/components/FormsList.vue (1)
314-344: Well-implemented permalink warning with minor suggestions.The warning notification is well-structured with proper internationalization, accessibility, and user experience considerations. A few minor improvements could enhance robustness:
Consider adding defensive checks for the imported constants:
- <div v-if="isPlainPermalink" class="wpuf-bg-yellow-50 wpuf-border wpuf-border-yellow-200 wpuf-rounded-md wpuf-p-4 wpuf-mt-6"> + <div v-if="isPlainPermalink && permalinkUrl" class="wpuf-bg-yellow-50 wpuf-border wpuf-border-yellow-200 wpuf-rounded-md wpuf-p-4 wpuf-mt-6">This ensures the warning only shows when both the condition is true AND the settings URL is available, preventing potential broken links if the backend data is incomplete.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
assets/js/forms-list.min.jsis excluded by!**/*.min.jsassets/js/forms-list.min.js.mapis excluded by!**/*.map,!**/*.min.js.map
📒 Files selected for processing (10)
admin/form-builder/assets/js/components/field-multiselect/index.js(1 hunks)admin/form-builder/assets/js/components/field-multiselect/template.php(1 hunks)admin/form-builder/assets/js/form-builder.js(1 hunks)assets/css/admin/form-builder.css(3 hunks)assets/js-templates/form-components.php(1 hunks)assets/js/components/FormsList.vue(2 hunks)assets/js/wpuf-form-builder-components.js(1 hunks)assets/js/wpuf-form-builder.js(1 hunks)includes/Admin/Menu.php(1 hunks)includes/Ajax/Admin_Form_Builder_Ajax.php(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
admin/form-builder/assets/js/components/field-multiselect/index.js
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 42-42: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
assets/js/wpuf-form-builder-components.js
[error] 457-457: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 461-461: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 461-461: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 464-464: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 467-467: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 502-502: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Run PHPCS inspection
🔇 Additional comments (12)
assets/js-templates/form-components.php (1)
295-295: LGTM! Template correctly uses dynamic options.The change from
option_field.optionstodynamic_optionsproperly aligns with the new dynamic taxonomy loading functionality, enabling the multi-select dropdown to display taxonomy terms dynamically based on the selected post type.admin/form-builder/assets/js/components/field-multiselect/template.php (1)
16-16: LGTM! Consistent with dynamic options implementation.The template correctly uses
dynamic_optionsfor option iteration, maintaining consistency with the Vue component's computed property that generates taxonomy options dynamically.includes/Ajax/Admin_Form_Builder_Ajax.php (3)
154-164: Good addition: Preserving existing form settings.The logic to retrieve and preserve existing form settings ensures that saved taxonomy selections are maintained when the form is loaded. The null checks and array validation provide good defensive programming.
181-193: Excellent security practices with proper escaping.The updated markup includes proper HTML escaping with
esc_attr()andesc_html()functions, and the addition of data attributes will support the JavaScript functionality for dynamic option loading.
197-202: Proper handling of selected options and error checking.The code correctly checks for WP_Error, handles empty categories gracefully, and preserves existing selections by marking appropriate options as selected. The HTML escaping ensures security.
admin/form-builder/assets/js/components/field-multiselect/index.js (1)
68-75: Good reactive behavior with watcher.The watcher on
dynamic_optionsensures the selectize control is refreshed when options change, providing proper reactivity. The use of$nextTickensures DOM updates are complete before rebinding.assets/js/wpuf-form-builder-components.js (1)
487-494: Reactive selectize updates enhance user experience.The watcher implementation correctly responds to dynamic option changes and properly uses
$nextTickto ensure DOM updates are complete before rebinding selectize.assets/js/wpuf-form-builder.js (1)
1113-1126: Proper initialization ensures categories load on page load.The document ready handler correctly initializes default categories when the page loads and properly binds change events. The use of a flag to distinguish initial load from user changes is a good practice.
admin/form-builder/assets/js/form-builder.js (2)
1157-1167: Excellent Selectize initialization with proper lifecycle management.The approach of checking for non-selectized elements (
:not(.selectized)) prevents double-initialization and relies on server-rendered selected attributes, which is a clean separation of concerns.
1146-1147: 🛠️ Refactor suggestionPotential browser compatibility issue with :has() selector.
The CSS
:has()pseudo-class in the selector might not be supported in older browsers, which could cause the removal logic to fail silently.Replace the
:has()selector with a more compatible approach:-$('.taxonomy-container, .wpuf_settings_taxonomy, .wpuf-input-container:has(select[name*="default_"])').remove(); +$('.taxonomy-container, .wpuf_settings_taxonomy').remove(); +$('.wpuf-input-container').filter(function() { + return $(this).find('select[name*="default_"]').length > 0; +}).remove();Likely an incorrect or invalid review comment.
includes/Admin/Menu.php (1)
129-133: Added plain permalink indicator and settings URL to localized script
The newis_plain_permalinkflag andpermalink_settings_urlentries will let the Vue component surface a warning when permalinks are set to “Plain.” WordPress’s JSON encoding will safely escape these values.
Please verify by switching your permalinks to “Plain” in a test site and confirming that the admin warning appears with the correct link to the Permalink Settings page.assets/js/components/FormsList.vue (1)
11-12: LGTM: Clean constant imports from global object.The imports are straightforward and follow the existing pattern in the component.
| function populate_default_categories(obj, isInitialLoad) { | ||
| var post_type = $(obj).val(); | ||
|
|
||
| // Don't proceed if no post type is selected | ||
| if (!post_type) { | ||
| return; | ||
| } | ||
|
|
||
| // Get form ID from the form | ||
| var form_id = $('input[name="wpuf_form_id"]').val() || 0; | ||
|
|
||
| wp.ajax.send('wpuf_form_setting_post', { | ||
| data: { | ||
| post_type: post_type, | ||
| form_id: form_id, | ||
| wpuf_form_builder_setting_nonce: wpuf_form_builder.nonce | ||
| }, | ||
| success: function (response) { | ||
| const default_category = 'select#default_category'; | ||
| let default_category_name = default_category; | ||
|
|
||
| if ( post_type !== 'post' ) { | ||
| default_category_name = 'select#default_' + post_type + '_cat'; | ||
| } | ||
|
|
||
| const value = $(default_category_name).data('value'); | ||
|
|
||
| $(default_category).parent('.wpuf-my-4.wpuf-input-container').remove(); | ||
| $('select#post_type').parent('.wpuf-my-4.wpuf-input-container').after(response.data); | ||
|
|
||
| if (value && ( typeof value === 'string' )) { | ||
| $(default_category).val(value.split(",")); | ||
| } else { | ||
| $(default_category).val(value); | ||
| // Remove all existing taxonomy containers | ||
| $('.taxonomy-container, .wpuf_settings_taxonomy, .wpuf-input-container:has(select[name*="default_"])').remove(); | ||
|
|
||
| // Find the container to append new content after | ||
| var $container = $(obj).closest('.wpuf-input-container'); | ||
|
|
||
| if ($container.length && response.data) { | ||
| // Append the new taxonomy fields | ||
| $container.after(response.data); | ||
|
|
||
| // Initialize selectize for all new taxonomy selects | ||
| $('.tax-list-selector:not(.selectized)').each(function() { | ||
| var $select = $(this); | ||
|
|
||
| // The select options are already set with selected attributes from PHP | ||
| // so we don't need to manually set values here | ||
|
|
||
| // Initialize selectize | ||
| $select.selectize({ | ||
| plugins: ['remove_button'], | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| $(default_category).selectize({ | ||
| plugins: ['remove_button'], | ||
| }); | ||
|
|
||
| }, | ||
| error: function ( error ) { | ||
| console.log(error); | ||
| error: function (error) { | ||
| console.log('Error loading default categories:', error); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhanced category population function improves taxonomy field management.
The refactored populate_default_categories function includes several important improvements:
- Early exit for empty post types prevents unnecessary AJAX calls
- Form ID inclusion ensures proper context for taxonomy loading
- Comprehensive cleanup removes all existing taxonomy containers
- Proper selectize initialization for dynamically added fields
However, the selector for removing existing containers might be too broad and could accidentally remove unrelated elements:
success: function (response) {
- // Remove all existing taxonomy containers
- $('.taxonomy-container, .wpuf_settings_taxonomy, .wpuf-input-container:has(select[name*="default_"])').remove();
+ // Remove existing taxonomy containers more specifically
+ $('.taxonomy-container, .wpuf_settings_taxonomy').remove();
+ // Remove only taxonomy-related default selects
+ $('.wpuf-input-container').has('select[name*="default_"][name*="tax"]').remove();
// Find the container to append new content after
var $container = $(obj).closest('.wpuf-input-container');This change makes the cleanup more targeted to avoid accidentally removing unrelated form elements.
🤖 Prompt for AI Agents
In assets/js/wpuf-form-builder.js around lines 1128 to 1174, the current
selector used to remove existing taxonomy containers is too broad and risks
removing unrelated elements. Refine the selector in the cleanup step to target
only the taxonomy containers related to the current form or context, such as
limiting it to descendants of the current form or container, to avoid
accidentally removing unrelated form elements.
| function populate_default_categories(obj, isInitialLoad) { | ||
| var post_type = $(obj).val(); | ||
|
|
||
| // Don't proceed if no post type is selected | ||
| if (!post_type) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good parameter addition but verify usage.
The new isInitialLoad parameter is a good addition for distinguishing between initial load and user interactions. However, this parameter doesn't appear to be used in the function logic.
Verify if the isInitialLoad parameter should influence the function behavior:
🏁 Script executed:
#!/bin/bash
# Search for any conditional logic based on isInitialLoad parameter
rg -A 10 -B 5 "isInitialLoad" --type jsLength of output: 2188
Remove or Implement the isInitialLoad Parameter
The isInitialLoad argument in populate_default_categories is currently unused—no conditional logic checks it. This can be confusing and should be addressed:
• In admin/form-builder/assets/js/form-builder.js (line 1128)
• In assets/js/wpuf-form-builder.js (where the same function is defined)
Either:
- Remove the
isInitialLoadparameter from the function signature and all calls, or - Implement logic inside populate_default_categories (e.g.,
if (isInitialLoad) { … } else { … }) and ensure you call it withtrueon initial page load.
This will keep the code clear and avoid dead parameters.
🤖 Prompt for AI Agents
In admin/form-builder/assets/js/form-builder.js around lines 1128 to 1134, the
newly added parameter isInitialLoad in the populate_default_categories function
is not used anywhere in the function body, causing confusion. To fix this,
either remove the isInitialLoad parameter from the function signature and all
calls to this function, or implement conditional logic inside the function that
uses isInitialLoad to differentiate behavior on initial load versus user
interaction, ensuring calls pass true during initial page load if applicable.
Rubaiyat-E-Mohammad
added
QA In Progress
QA Approved
…l#1577) * fix: custom taxonomies are not selectable * permalink notice
2 participants
closes #1529
Product categories are not displayed/selectable in a post form of type "product", instead only post categories are displayed!
Summary by CodeRabbit