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

PCH Smart Linking: Fix permission check typo #2651

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

vaurdan
Copy link
Contributor

@vaurdan vaurdan commented Jul 17, 2024

Description

This PR fixes a typo from #2649, where the last return value is false, where it should be true.

Motivation and context

Fix the Smart Linking functionality

How has this been tested?

Tested locally, with a user with two roles.

Summary by CodeRabbit

  • New Features

    • Enhanced the SmartLink class to dynamically set the title based on the destination_post_id.
  • Refactor

    • Improved code readability and maintainability in the current_user_can_use_pch_feature function by restructuring user role and capability checks.

@vaurdan vaurdan added this to the 3.16.1 milestone Jul 17, 2024
@vaurdan vaurdan requested a review from a team as a code owner July 17, 2024 14:48
Copy link
Contributor

coderabbitai bot commented Jul 17, 2024

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Base branches to auto review (4)
  • develop
  • add/.*
  • fix/.*
  • update/.*

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Walkthrough

The recent updates enhance the SmartLink and PermissionHandler classes to improve flexibility and readability. The SmartLink constructor now dynamically sets the title property based on the destination_post_id. In PermissionHandler, the current_user_can_use_pch_feature function has been refactored for better clarity and maintainability, changing how it handles user role and capability checks.

Changes

File Change Summary
src/Models/class-smart-link.php The __construct method now conditionally sets the title property based on the destination_post_id.
src/class-permissions.php Refactored the current_user_can_use_pch_feature function for improved readability and maintainability.

Sequence Diagrams

Dynamic Title Setting in SmartLink

sequenceDiagram
    participant User
    participant SmartLink
    participant WordPress

    User ->> SmartLink: Instantiate SmartLink with destination_post_id
    SmartLink ->> WordPress: Call get_the_title(destination_post_id)
    WordPress -->> SmartLink: Return post title
    SmartLink ->> SmartLink: Set title property
    SmartLink -->> User: SmartLink object with title set
Loading

Role and Capability Validation in PermissionHandler

sequenceDiagram
    participant User
    participant PermissionHandler
    participant WordPress

    User ->> PermissionHandler: Call current_user_can_use_pch_feature(user_roles)
    PermissionHandler ->> WordPress: Retrieve user roles
    WordPress -->> PermissionHandler: Return user roles
    PermissionHandler ->> PermissionHandler: Check roles and capabilities
    PermissionHandler -->> User: Return true/false
Loading

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@vaurdan
Copy link
Contributor Author

vaurdan commented Jul 17, 2024

@acicovic not adding a label here, since we don't want this to show up on the changelog.

@vaurdan vaurdan self-assigned this Jul 17, 2024
@vaurdan vaurdan changed the base branch from develop to prepare/3.16.1 July 17, 2024 14:49
@vaurdan vaurdan force-pushed the fix/permission-check-typo branch from d51b64d to 8ef7574 Compare July 17, 2024 14:49
Copy link
Collaborator

@acicovic acicovic left a comment

Choose a reason for hiding this comment

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

Good catch.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
src/class-permissions.php (1)

Line range hint 14-30: Review of get_user_roles_with_edit_posts_cap function.

This function retrieves all user roles that have the edit_posts capability. The implementation uses WordPress functions and iterates over roles to check capabilities. The function is straightforward and adheres to WordPress coding standards.

Potential Improvement:
Consider caching the result to improve performance, especially if this function is called frequently.

+ $cached_roles = wp_cache_get('user_roles_with_edit_posts_cap');
+ if ($cached_roles) {
+    return $cached_roles;
+ }
  $result = array();
  $roles  = wp_roles()->roles;
  foreach ( $roles as $key => $role ) {
      if ( isset( $role['capabilities']['edit_posts'] ) ) {
          $result[ $key ] = $role['name'];
      }
  }
+ wp_cache_set('user_roles_with_edit_posts_cap', $result);
  return $result;
Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 990a1a5 and 8ef7574.

Files selected for processing (2)
  • src/Models/class-smart-link.php (1 hunks)
  • src/class-permissions.php (1 hunks)
Additional context used
Path-based instructions (2)
src/class-permissions.php (1)

Pattern **/*.{html,php}: "Perform a detailed review of the provided code with following key aspects in mind:

  • Review the HTML and PHP code to ensure it is well-structured and adheres to best practices.
  • Ensure the code follows WordPress coding standards and is well-documented.
  • Confirm the code is secure and free from vulnerabilities.
  • Optimize the code for performance, removing any unnecessary elements.
  • Validate comments for accuracy, currency, and adherence to WordPress coding standards.
  • Ensure each line comment concludes with a period.
  • Verify code compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/Models/class-smart-link.php (1)

Pattern **/*.{html,php}: "Perform a detailed review of the provided code with following key aspects in mind:

  • Review the HTML and PHP code to ensure it is well-structured and adheres to best practices.
  • Ensure the code follows WordPress coding standards and is well-documented.
  • Confirm the code is secure and free from vulnerabilities.
  • Optimize the code for performance, removing any unnecessary elements.
  • Validate comments for accuracy, currency, and adherence to WordPress coding standards.
  • Ensure each line comment concludes with a period.
  • Verify code compatibility with the latest version of WordPress, avoiding deprecated functions or features."
Additional comments not posted (2)
src/class-permissions.php (1)

84-110: Review of current_user_can_use_pch_feature function.

This function checks if the current user has the necessary permissions to access specific features. The logic has been refactored to improve readability and maintainability. The function now checks for user roles and capabilities more efficiently.

Observations:

  • The function retrieves user roles and checks them against valid roles with the capability to edit posts.
  • It also checks if the user's roles are allowed to access the specific feature or post.
  • Finally, it checks if the user can edit the post if a post ID is provided.

Potential Issue:
Ensure that the current_user_can function call correctly handles all scenarios, especially with custom post types or permissions.

src/Models/class-smart-link.php (1)

131-138: Review of SmartLink constructor.

The constructor now conditionally sets the title of the link based on whether the destination_post_id is set. If it is set, it retrieves the title of the destination post; otherwise, it uses the provided title.

Observations:

  • This change enhances flexibility by allowing dynamic title setting based on the destination post.
  • The use of get_the_title is appropriate here and follows WordPress standards.

Potential Issue:
Ensure that get_the_title does not lead to any performance issues, especially when dealing with a large number of posts.

Verification successful

Verification Successful: No performance issues found with get_the_title.

The usage of get_the_title in the reviewed files does not indicate any performance concerns, as it is not used within loops or with large data sets.

  • src/Models/class-smart-link.php: Used conditionally, not in a loop.
  • src/Metadata/class-page-for-posts-builder.php: Used once to set a metadata value.
  • src/Metadata/class-page-builder.php: Used once to set a metadata value.
  • src/Metadata/class-post-builder.php: Used once to set a metadata value.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the performance of `get_the_title` when used in loops or with many posts.

# Test: Search for the usage of `get_the_title` in loops or with large data sets.
rg --type php -A 5 $'get_the_title'

Length of output: 1391

@vaurdan vaurdan merged commit e7868db into prepare/3.16.1 Jul 17, 2024
33 of 52 checks passed
@vaurdan vaurdan deleted the fix/permission-check-typo branch July 17, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants