Beta 2 bugfixes 2 #446
Beta 2 bugfixes 2 #446
Conversation
|
Warning CodeRabbit GitHub Action detectedThe repository is using both CodeRabbit Pro and CodeRabbit Open Source (via GitHub Actions), which is not recommended as it may lead to duplicate comments and extra noise. Please remove the CodeRabbit GitHub Action. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
|
WalkthroughThis update enhances the LSX Tour Operators codebase by introducing a new block variation, refining CSS styling, and improving currency field handling in the Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (9)
- assets/css/scss/_icons.scss (2)
- assets/css/scss/style.scss (1)
- assets/css/style.css (3)
- assets/js/blocks/general.js (1)
- assets/js/blocks/tour.js (4)
- includes/classes/blocks/class-bindings.php (7)
- includes/classes/blocks/class-registration.php (9)
- includes/classes/legacy/class-accommodation.php (2)
- includes/classes/legacy/class-tour.php (1)
Files not summarized due to errors (2)
- assets/js/blocks/tour.js (diff tokens exceeds limit)
- includes/classes/blocks/class-registration.php (diff tokens exceeds limit)
Files not reviewed due to errors (1)
- includes/classes/blocks/class-registration.php (no response)
Files skipped from review due to trivial changes (5)
- assets/css/scss/_icons.scss
- assets/css/scss/style.scss
- assets/css/style.css
- assets/js/blocks/general.js
- includes/classes/legacy/class-accommodation.php
Review comments generated (12)
- Review: 8
- LGTM: 4
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| $currency_fields = [ | ||
| 'price', | ||
| 'single_supplement' | ||
| ]; | ||
|
|
||
| if ( get_post_type() === 'tour' && in_array( $meta_key, $currency_fields ) ) { |
There was a problem hiding this comment.
The changes made here are good for maintainability and flexibility. The code now supports multiple currency fields, which is a significant improvement over the previous version where only the 'price' field was supported. This change allows for easy addition of more currency fields in the future if needed. However, it's important to ensure that all fields listed in $currency_fields array are indeed intended to be treated as currency fields.
- if ( get_post_type() === 'tour' && 'price' === $meta_key ) {
+ $currency_fields = [
+ 'price',
+ 'single_supplement'
+ ];
+
+ if ( get_post_type() === 'tour' && in_array( $meta_key, $currency_fields ) ) {This change also improves the readability of the code by making it clear what fields are considered currency fields.
| [ | ||
| 'core/paragraph', { | ||
| metadata: { | ||
| bindings: { | ||
| content: { | ||
| source: 'lsx/post-meta', | ||
| args: { key: 'included' } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| [] | ||
| ] | ||
| fontSize: 'x-small', | ||
| content: '<strong>Single supplement:</strong>' | ||
| }] | ||
| ] | ||
| ], | ||
| [ | ||
| 'core/column', { | ||
| width: '50%', | ||
| className: 'lsx-not-included-wrapper', | ||
| ], | ||
| ['core/group', { | ||
| style: { | ||
| spacing: { | ||
| blockGap: '0' | ||
| } | ||
| spacing: { | ||
| blockGap: '5px' | ||
| }, | ||
| layout: { | ||
| type: 'flex', | ||
| flexWrap: 'nowrap' | ||
| } | ||
| } | ||
| }, | ||
| [ | ||
| [ | ||
| 'core/paragraph', { | ||
| style: { | ||
| elements: { | ||
| link: { | ||
| color: { | ||
| text: 'var:preset|color|primary-700' | ||
| ['core/paragraph', { | ||
| metadata: { | ||
| bindings: { | ||
| content: { | ||
| source: 'lsx/post-meta', | ||
| args: { | ||
| key: 'single_supplement' | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| className: 'amount has-primary-color has-text-color has-link-color', | ||
| style: { | ||
| elements: { | ||
| link: { | ||
| color: { | ||
| text: 'var:preset|color|primary-700' | ||
| } | ||
| } | ||
| }, | ||
| spacing: { | ||
| padding: { | ||
| top: '2px', | ||
| bottom: '2px' | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| textColor: 'primary-700', | ||
| fontSize: 'medium' | ||
| }, | ||
| [ 'Price Excludes:' ] | ||
| ], | ||
| [ | ||
| 'core/paragraph', { | ||
| metadata: { | ||
| bindings: { | ||
| content: { | ||
| source: 'lsx/post-meta', | ||
| args: { key: 'not_included' } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| [] | ||
| ] | ||
| ] | ||
| content: '' | ||
| }] | ||
| ] | ||
| ] | ||
| ] | ||
| ] | ||
| } ); | ||
| }); |
There was a problem hiding this comment.
The block variation 'lsx-tour-operator/single-supplement-wrapper' is being registered here. It seems to be well defined with appropriate attributes and innerBlocks. However, the image URL at line 201 is hardcoded which might cause issues if the file location changes or the file gets deleted. Consider storing such URLs in a configuration file or database and reference them from there.
- url: 'https://tour-operator.lsx.design/wp-content/uploads/2024/11/single-supplement-icon-black-52px-1.svg',
+ url: config.singleSupplementIconUrl,| @@ -326,7 +298,7 @@ | |||
| [ | |||
| 'core/image', | |||
| { | |||
| width: 20, | |||
| width: '20px', | |||
There was a problem hiding this comment.
The width property of the image has been changed from a number to a string. This could potentially cause issues if the consuming code expects a number. Ensure that this change is compatible with all parts of your application that use this property.
| border: { | ||
| radius: '8px', | ||
| width: '1px' | ||
| } | ||
| }, | ||
| backgroundColor: 'base', | ||
| borderColor: 'primary', | ||
| layout: { | ||
| type: 'constrained' | ||
| } | ||
| }, | ||
| innerBlocks: [ | ||
| [ 'core/columns', { | ||
| align: 'wide', | ||
| style: { | ||
| spacing: { | ||
| blockGap: { | ||
| top: 'var:preset|spacing|medium', | ||
| left: 'var:preset|spacing|medium' | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| [ | ||
| ['core/column', { | ||
| width: '50%', | ||
| id: 'lsx-included-wrapper', | ||
| style: { | ||
| spacing: { | ||
| blockGap: '0' | ||
| } | ||
| } | ||
| }, | ||
| [ | ||
| [ 'core/paragraph', { | ||
| style: { | ||
| elements: { | ||
| link: { | ||
| color: { | ||
| text: 'var:preset|color|primary-700' | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| textColor: 'primary-700', | ||
| fontSize: 'medium', | ||
| content: '<strong>Price Includes:</strong>' | ||
| } ], | ||
| [ 'core/paragraph', { | ||
| metadata: { | ||
| bindings: { | ||
| content: { | ||
| source: 'lsx/post-meta', | ||
| args: { key: 'included' } | ||
| } | ||
| } | ||
| } | ||
| }] | ||
| ] | ||
| ], | ||
| [ 'core/column', { | ||
| width: '50%', | ||
| className: 'lsx-not-included-wrapper', | ||
| style: { | ||
| spacing: { | ||
| blockGap: '0' | ||
| } | ||
| } | ||
| }, | ||
| [ | ||
| [ 'core/paragraph', { | ||
| style: { | ||
| elements: { | ||
| link: { | ||
| color: { | ||
| text: 'var:preset|color|primary-700' | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| textColor: 'primary-700', | ||
| fontSize: 'medium', | ||
| content: '<strong>Price Excludes:</strong>' | ||
| }], | ||
| [ 'core/paragraph', { | ||
| metadata: { | ||
| bindings: { | ||
| content: { | ||
| source: 'lsx/post-meta', | ||
| args: { key: 'not_included' } | ||
| } | ||
| } | ||
| } | ||
| }] | ||
| ] | ||
| ] | ||
| ] | ||
| ] | ||
| ] | ||
| } ); |
There was a problem hiding this comment.
The block variation 'lsx-tour-operator/price-include-exclude' is being registered here. It seems to be well defined with appropriate attributes and innerBlocks. However, similar to the previous comment, consider storing URLs in a configuration file or database and reference them from there.
| if ( null !== $value ) { | ||
| $date_transforms = [ | ||
| 'booking_validity_start', | ||
| 'booking_validity_end', | ||
| ]; | ||
| if ( in_array( $source_args['key'], $date_transforms ) ) { | ||
| $value = wp_date( 'j M Y', $value ); | ||
| } | ||
|
|
||
| $value = preg_replace( '/^<p>(.*?)<\/p>$/', '$1', $value ); | ||
| } | ||
|
|
||
| $value = preg_replace( '/^<p>(.*?)<\/p>$/', '$1', $value ); | ||
|
|
There was a problem hiding this comment.
The check for null value before transforming the date and removing paragraph tags is a good addition. However, it's important to note that wp_date() function uses the timezone set in WordPress settings. If you want to use GMT time, consider using gmdate() instead.
| if ( empty( $value ) ) { | ||
| $value = ''; | ||
| } | ||
| $value = '<div class="' . $classes . '"/>' . $value . '</div>'; |
There was a problem hiding this comment.
This change improves the code by ensuring that an empty string is assigned to $value if it is indeed empty. This prevents potential issues down the line where $value might be used expecting a string but gets a different data type.
| if ( ! is_array( $gallery ) ) { | ||
| $gallery = [ $gallery ]; | ||
| } | ||
|
|
There was a problem hiding this comment.
This change ensures that $gallery is always an array, which can prevent errors when trying to iterate over $gallery later in the code.
| return $block_content; | ||
| } |
There was a problem hiding this comment.
The removal of the maybe_hide_varitaion function seems to be a significant change. Ensure that this functionality is either no longer needed or has been implemented elsewhere. If not, this could lead to unexpected behavior.
There was a problem hiding this comment.
Files selected (1)
- includes/classes/blocks/class-registration.php (8)
Files not summarized due to errors (1)
- includes/classes/blocks/class-registration.php (diff tokens exceeds limit)
Review comments generated (7)
- Review: 3
- LGTM: 4
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| add_filter( 'render_block', array( $this, 'maybe_hide_varitaion' ), 10, 3 ); | ||
|
|
||
| add_filter( 'posts_pre_query', array( $this, 'posts_pre_query' ), 10, 2 ); |
There was a problem hiding this comment.
The new filter render_block is added here. Please ensure that the function maybe_hide_varitaion (which seems to have a typo, should be maybe_hide_variation) handles all edge cases and doesn't break any existing functionality.
| do_action( 'qm/debug', [ $key ] ); | ||
|
|
There was a problem hiding this comment.
This line of code seems to be for debugging purposes. Make sure to remove it before merging to avoid unnecessary logging in production.
| } | ||
| } else if ( taxonomy_exists( $key ) ) { | ||
| // Check to see if this is a taxonomy or a custom field. | ||
| $tax_args = array( | ||
| 'fields' => 'ids' | ||
| ); | ||
| if ( empty( wp_get_post_terms( get_the_ID(), $key, $tax_args ) ) ) { | ||
| $block_content = ''; | ||
| } | ||
| } else { | ||
| $key = str_replace( '-', '_', $key ); | ||
| $key_array = [ $key ]; | ||
| $has_values = false; | ||
|
|
||
| // If this is a wrapper that houses many fields, then we need to review them all. | ||
| if ( 'include_exclude' === $key ) { | ||
| $key_array = [ 'included', 'not_included' ]; | ||
| } | ||
|
|
||
| foreach ( $key_array as $meta_key ) { | ||
| $value = lsx_to_custom_field_query( $meta_key, '', '', false ); | ||
|
|
||
| // we need to see if the posts exist before we can use them | ||
| if ( stripos( $meta_key, '_to_' ) && 0 === $this->post_ids_exist( $value ) ) { | ||
| continue; | ||
| } | ||
|
|
||
| if ( ! empty( $value ) && '' !== $value ) { | ||
| $has_values = true; | ||
| } | ||
| } | ||
|
|
||
| if ( false === $has_values ) { | ||
| $block_content = ''; | ||
| } | ||
| } | ||
|
|
||
| return $block_content; | ||
| } | ||
|
|
||
| /** | ||
| * Determines if a post exists based on the ID. | ||
| * | ||
| * | ||
| * @global wpdb $wpdb WordPress database abstraction object. | ||
| * | ||
| * @param string $title Post title. | ||
| * @param string $content Optional. Post content. | ||
| * @param string $date Optional. Post date. | ||
| * @param string $type Optional. Post type. | ||
| * @param string $status Optional. Post status. | ||
| * @return int Post ID if post exists, 0 otherwise. | ||
| */ | ||
| protected function post_ids_exist( $ids ) { | ||
| global $wpdb; | ||
|
|
||
| if ( is_array( $ids ) ) { | ||
| $ids = implode( ',', $ids ); | ||
| } | ||
|
|
||
| $ids = wp_unslash( sanitize_post_field( 'id', $ids, 0, 'db' ) ); | ||
|
|
||
| $query = "SELECT COUNT(ID) | ||
| FROM $wpdb->posts | ||
| WHERE 1=1 | ||
| AND ID IN (%s) | ||
| AND post_status IN ('draft', 'publish')"; | ||
|
|
||
| return (int) $wpdb->get_var( $wpdb->prepare( $query, $ids ) ); | ||
| } | ||
|
|
||
| /** | ||
| * This function will grab our Featured query so we dont have to redo that. | ||
| * | ||
| * @param null $posts | ||
| * @param WP_Query $query | ||
| * @return null|array | ||
| */ | ||
| public function posts_pre_query( $posts, $query ) { | ||
| if ( isset( $query->query['lsx_to_featured'] ) && isset( $this->featured[ $query->query['lsx_to_featured'] ] ) ) { | ||
| do_action( 'qm/debug', $this->featured[ $query->query['lsx_to_featured'] ] ); | ||
| $posts = $this->featured[ $query->query['lsx_to_featured'] ]; | ||
| } | ||
| return $posts; | ||
| } | ||
|
|
||
| /** | ||
| * Find the featured items for the current query | ||
| * | ||
| * @param array $query | ||
| * @return array | ||
| */ | ||
| public function find_featured_items( $query ) { | ||
| $items = []; | ||
| $item_query = new \WP_Query( $query ); | ||
| if ( $item_query->have_posts() ) { | ||
| $items = $item_query->posts; | ||
| } | ||
| return $items; | ||
| } |
There was a problem hiding this comment.
This is a large block of code added to handle block variations and their visibility based on certain conditions. It's hard to review without full context, but make sure to thoroughly test this logic with different scenarios to ensure it works as expected. Also, consider breaking down this large function into smaller ones to improve readability and maintainability.
Also, there's a typo in the function name maybe_hide_varitaion, it should be maybe_hide_variation.
- public function maybe_hide_varitaion( $block_content, $parsed_block, $block_obj ) {
+ public function maybe_hide_variation( $block_content, $parsed_block, $block_obj ) {In line 383, the function post_ids_exist is defined. This function checks if a post exists based on its ID. However, the docstring does not match the function's purpose. Please update the docstring to accurately reflect what the function does.
- * Determines if a post exists based on the ID.
- *
- *
- * @global wpdb $wpdb WordPress database abstraction object.
- *
- * @param string $title Post title.
- * @param string $content Optional. Post content.
- * @param string $date Optional. Post date.
- * @param string $type Optional. Post type.
- * @param string $status Optional. Post status.
- * @return int Post ID if post exists, 0 otherwise.
+ * Determines if posts exist based on their IDs.
+ *
+ * @global wpdb $wpdb WordPress database abstraction object.
+ *
+ * @param array|string $ids Post IDs.
+ * @return int Count of existing posts.
Description
The following fixes were added to the beta 2 version. From the DBA and the TO bugherd board respectively.
Tasks
Resources
This branch is being deployed to
Summary by CodeRabbit
price_filtermethod, improving support for multiple currency fields..lsx-single-supplement-wrapperand added currency icons for different currencies, improving visual representation of prices.