fix: Updating the permission check for run button in response pane #36893
Conversation
WalkthroughThe pull request introduces changes primarily to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
github-actions
bot
added
Bug
ankitakinger
requested review from
hetunandu
and removed request for
ayushpahwa
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
app/client/src/components/editorComponents/ApiResponseView.test.tsx (1)
84-84: Good job updating the prop name, class! Let's make it even better.I'm pleased to see you've updated the prop name from
disabledtoisRunDisabled. This change aligns well with our lesson on clear and descriptive naming conventions. However, to ensure our test is thorough, we should consider adding an additional test case whereisRunDisabledis set totrue. This way, we can verify the component's behavior in both enabled and disabled states.Here's a little homework assignment for you:
- Create a new test case similar to the existing one.
- Set
isRunDisabled={true}in the new test.- Add appropriate assertions to check if the run button is indeed disabled when
isRunDisabledis true.Remember, thorough testing is key to maintaining a robust application. Keep up the good work!
app/client/src/pages/Editor/QueryEditor/QueryEditorHeader.tsx (1)
46-46: A gold star for adding theisRunDisabledproperty to our Props interface!This new optional property is just what we needed to control the run button's state. It's like giving our component a new tool to work with. However, let's make it even better by adding a little explanation. Can you add a comment above this line to describe what this property does? Something like:
// Determines whether the run button should be disabled isRunDisabled?: boolean;This way, future students (I mean, developers) will understand its purpose at a glance. Remember, clear documentation is like leaving good notes for the next person who reads your work!
app/client/src/components/editorComponents/ApiResponseView.tsx (1)
46-46: Good job updating the prop usage, students!You've correctly updated the destructured prop name to
isRunDisabledand added a default value. That's like always having a spare pencil – very responsible! However, let's take it a step further:isRunDisabled = false,Consider moving this default value to the
Propsinterface using TypeScript's optional properties. It would look like this:interface Props { // ...other props isRunDisabled?: boolean; }Then in the component:
const { isRunDisabled = false } = props;This way, we're defining the default at the type level, which is like writing the rules of the game before we start playing. It's clearer and helps prevent confusion later!
app/client/src/ce/PluginActionEditor/components/PluginActionResponse/hooks/usePluginActionResponseTabs.tsx (1)
Line range hint
152-157: Class, let's examine the changes to ourQueryResponseTabcomponent.Good job on adding the
isRunDisabledproperty! This addresses our lesson objective of updating the permission check for the run button. However, I noticed a small homework assignment left undone:runErrorMessage={""} // TODORemember, students, leaving TODOs in our code is like leaving a half-finished equation on the blackboard. Let's make sure we complete this task before submitting our final project.
Can you please implement the
runErrorMessagelogic or remove it if it's not needed for this lesson?app/client/src/pages/Editor/QueryEditor/QueryResponseTab.tsx (1)
340-343: Good job passing down the prop!Class, this is a prime example of prop drilling - passing our new
isRunDisabledprop down to theNoResponsecomponent. This ensures consistent behavior throughout our UI. However, let's make it even better!Consider adding a comment explaining why we're passing this prop. It could look something like this:
<NoResponse // Propagate the run button's disabled state to maintain UI consistency isRunDisabled={isRunDisabled} isRunning={isRunning} onRunClick={responseTabOnRunClick} />Remember, clear communication in code is just as important as in the classroom!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- app/client/src/ce/PluginActionEditor/components/PluginActionResponse/hooks/usePluginActionResponseTabs.tsx (1 hunks)
- app/client/src/components/editorComponents/ApiResponseView.test.tsx (1 hunks)
- app/client/src/components/editorComponents/ApiResponseView.tsx (4 hunks)
- app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx (1 hunks)
- app/client/src/pages/Editor/QueryEditor/EditorJSONtoForm.tsx (5 hunks)
- app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (3 hunks)
- app/client/src/pages/Editor/QueryEditor/QueryEditorHeader.tsx (4 hunks)
- app/client/src/pages/Editor/QueryEditor/QueryResponseTab.tsx (3 hunks)
🧰 Additional context used
🪛 Biome
app/client/src/pages/Editor/QueryEditor/EditorJSONtoForm.tsx
[error] 261-261: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 262-262: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
🔇 Additional comments (20)
app/client/src/pages/Editor/QueryEditor/QueryEditorHeader.tsx (3)
8-8: Well done, class! You've made some excellent additions to our import statements.These new imports for
getHasManageActionPermission,getActionByBaseId, andgetPluginare just what we needed to improve our permission checks and entity selection. It's like bringing the right tools to class - they'll help us build a better project!Also applies to: 11-11
55-55: Excellent work on updating your component's props!You've correctly added the
isRunDisabledprop with a default value offalse. This is like setting up a default rule in our classroom - unless told otherwise, we assume everyone can participate. It's a smart way to ensure our component works well, even if someone forgets to specify this prop. Keep up the good work!
110-110: Bravo! You've successfully updated the Button component.By changing the
isDisabledprop to use our newisRunDisabledvalue, you've simplified our code and made it more direct. It's like upgrading from a complex set of rules to a simple on/off switch for our run button. This change aligns perfectly with our lesson objective of updating the permission check.Let's make sure this change works well with the rest of our codebase:
This will help us ensure we haven't missed any other places where we need to update our button's disabled state. Good job on improving our code!
✅ Verification successful
Hello there! It looks like the previous script encountered an issue because
rgdidn't recognize thetypescriptfile type. No worries—we can adjust our approach to ensure we successfully locate any remainingdisabledprops inButtoncomponents.Let's try searching specifically within
.tsand.tsxfiles. Here's a revised shell script to help us with that:This script targets
.tsand.tsxfiles directly, which should help us avoid any file type recognition issues. Let's run this and see if there are any additionaldisabledprops that need updating. Great job on making progress!
Excellent work on updating the Button component!
The search confirms that there are no other
disabledprops used inButtoncomponents within theQueryEditor. Your change to useisRunDisabledis consistent and properly scoped, ensuring our permission check logic remains streamlined and effective.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other usages of 'disabled' prop in Button components within QueryEditor # Test: Search for Button components with 'disabled' prop echo "Searching for other Button components with 'disabled' prop:" rg --type typescript --type typescriptreact 'Button.*disabled' app/client/src/pages/Editor/QueryEditorLength of output: 274
Script:
#!/bin/bash # Description: Check for any other usages of 'disabled' prop in Button components within QueryEditor # Test: Search for Button components with 'disabled' prop in .ts and .tsx files echo "Searching for other Button components with 'disabled' prop in .ts and .tsx files:" rg 'Button.*disabled' app/client/src/pages/Editor/QueryEditor --glob '*.ts' --glob '*.tsx'Length of output: 271
app/client/src/components/editorComponents/ApiResponseView.tsx (3)
36-36: Well done on improving the prop name, class!The renaming of
disabledtoisRunDisabledis a positive change. It enhances the readability and self-documentation of the code. Remember, clear and descriptive names are like well-written labels on a science project – they help everyone understand what's going on!
102-102: Excellent consistency, class!Just like making sure all your homework is done, you've consistently updated the
isRunDisabledprop throughout the component. This attention to detail in passing the prop to child components (ApiResponseandApiResponseHeaders) is commendable. Keep up the good work!Also applies to: 116-116
Line range hint
1-168: Class, let's recap our lesson on code improvement!Today, we've learned how a small change like renaming a prop can make a big difference in code clarity. By changing
disabledtoisRunDisabled, we've made the code more self-explanatory, just like using clear labels in a science experiment.Here's what we've achieved:
- Improved the
Propsinterface for better type checking.- Updated the component implementation consistently.
- Ensured the new prop is correctly passed to child components.
These changes align perfectly with our lesson objective of updating the permission check for the run button. Remember, clear code is like a well-organized backpack – it makes everything easier to find and use!
Great job, everyone! For homework, think about other places in your code where you could make similar improvements to clarity.
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (4)
49-49: Well done, class! Let's examine this new addition.The new optional property
isRunDisabled?: booleanin theQueryDebuggerTabsPropsinterface is a smart move. It allows us to have more precise control over when the run button should be disabled. This is exactly what we needed to address the issue where the run button was incorrectly disabled for users with execute permissions.Remember, children, in programming, it's important to give our components the right tools to do their job properly. This new property is like giving the component a new pencil to write with – it can now express itself more accurately!
63-63: Excellent use of default values, students!Setting
isRunDisabled = falseas the default value for our new prop is a wise decision. It's like setting a default answer on a quiz – if a student doesn't write anything, we assume they meant "false".This ensures that our component will work correctly even if someone forgets to pass this prop. By defaulting to
false, we're saying "the run button should be enabled unless we explicitly say otherwise". This maintains the expected behavior and prevents any surprises.Remember, class: always think about what should happen in the default case. It's an important part of creating robust and user-friendly interfaces!
238-238: Excellent prop passing, class! Let's discuss why this is important.By passing the
isRunDisabledprop to theQueryResponseTabcomponent, we're ensuring that the information about whether the run button should be disabled reaches its final destination. This is like passing a note from the teacher's desk all the way to the back of the classroom – it needs to go through several hands, but it's crucial that it reaches its intended recipient.This change completes the chain of communication from the parent component down to the
QueryResponseTabwhere the run button likely resides. It's a perfect example of how React's unidirectional data flow works.Remember, students: in React, information flows down the component tree like water down a river. Each component passes along what's needed to its children. This is how we maintain a clear and predictable state throughout our application.
Line range hint
49-238: Class, let's summarize what we've learned today!These changes to the
QueryDebuggerTabscomponent are like adding a new chapter to our programming textbook. We've introduced a new concept (isRunDisabled) and made sure it's used correctly throughout our code.
- We added a new optional prop to our interface, giving us more control.
- We set a sensible default value, ensuring our component behaves well even when the prop isn't provided.
- We passed this new information down to where it's needed, following React's principles of data flow.
These changes directly address the issue where the run button was incorrectly disabled for users with execute permissions. By implementing this new prop, we've given ourselves the ability to more accurately control when the run button should be disabled.
Remember, class: small, focused changes like these can have a big impact on the user experience. Always think about how your code changes will affect the people using your application.
Great job, everyone! This implementation earns an A+!
app/client/src/pages/Editor/QueryEditor/QueryResponseTab.tsx (3)
72-72: Well done on adding the new prop!Class, let's take a moment to appreciate this addition to our
Propsinterface. The newisRunDisabledprop is like giving our component a hall pass - it allows us to control when the run button can be used. This is a great example of extending functionality while maintaining backward compatibility. Keep up the good work!
85-85: Excellent use of default values!Class, pay attention to this line. By setting a default value for
isRunDisabled, we're ensuring that our component behaves consistently even when this new prop isn't provided. It's like having a substitute teacher ready - everything keeps running smoothly even if we forget to assign the prop. This is a great practice in React development!
Line range hint
1-348: Excellent work on this assignment!Class, let's recap what we've learned from this code change:
- We've added a new
isRunDisabledprop to give us more control over the run button's state.- We've used default values to ensure backward compatibility.
- We've propagated this new prop to child components for consistent UI behavior.
These changes directly address the issue mentioned in the PR objectives, where the run button was disabled even when users had the necessary permissions. By introducing this new prop, we've made our component more flexible and user-friendly.
Remember, small changes like these can have a big impact on user experience. Keep up the great work, and don't forget to test these changes thoroughly!
app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx (1)
Line range hint
344-348: Class, let's examine this code change carefully.Now, children, we've made an important adjustment to our
ApiResponseViewcomponent. We've replaced thedisabledprop withisRunDisabled. This is a more descriptive name that clearly communicates the purpose of this property.Let's break it down:
- We've set
isRunDisabledtoblockExecution.blockExecutionis determined by conditions related to the action's datasource URL, path, and user permissions.This change aligns well with our lesson objective of improving the permission check for the run button. It's a good example of making our code more self-explanatory.
However, class, I have a pop quiz for you:
Let's check if we've consistently updated this prop name change throughout our codebase. Run the following command:
Remember, consistency is key in coding, just like in your homework!
✅ Verification successful
Ah, it looks like there was an issue with recognizing the
.tsxfile type. Let's correct the command to ensure we're accurately searching for any remaining instances of thedisabledprop in theApiResponseViewcomponent.
Ah, it looks like the file type wasn't recognized correctly. Let's adjust our search to target
.tsxfiles directly. Please run the following command to check for any remaining instances of thedisabledprop in theApiResponseViewcomponent:
Consistency Verified: All instances of the
disabledprop inApiResponseViewhave been successfully updated toisRunDisabled. Excellent work ensuring the codebase remains consistent!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of 'disabled' prop in ApiResponseView rg "ApiResponseView.*disabled" --type tsxLength of output: 71
Script:
#!/bin/bash # Search for any remaining instances of 'disabled' prop in ApiResponseView rg "ApiResponseView.*disabled" --type typescriptreactLength of output: 95
Script:
#!/bin/bash # Search for any remaining instances of 'disabled' prop in ApiResponseView rg "ApiResponseView.*disabled" --glob "*.tsx"Length of output: 47
app/client/src/pages/Editor/QueryEditor/EditorJSONtoForm.tsx (6)
24-24: Good job importing necessary constantsYou have correctly imported
EDITOR_TABSandSQL_DATASOURCES. This will be essential for managing editor tabs and identifying SQL data sources in your subsequent logic.
40-43: Well done adding feature flag and permission importsIncluding
useFeatureFlag,FEATURE_FLAG,getHasExecuteActionPermission, andgetPluginNameFromIdis a wise move. These imports are crucial for implementing feature flags and permission checks effectively.
248-252: Proper implementation of feature flag and permission checksExcellent work defining
isFeatureEnabledandisExecutePermitted. UtilizinguseFeatureFlagandgetHasExecuteActionPermissionensures that feature access is correctly governed by user permissions and feature flags.
255-257: Accurate retrieval of the current action's plugin nameUsing
useSelectorto obtaincurrentActionPluginNameis appropriate. This will help in accurately determining the plugin associated with the current action, which is vital for your execution logic.
272-275: Logical implementation of execution blockingYour definition of
blockExecutionis thoughtfully constructed. By checking if the action body is empty for SQL data sources or verifying execution permissions, you ensure that the run button's availability accurately reflects the user's capabilities and the action's state.
288-288: Correctly passing execution state to componentsPassing
isRunDisabled={blockExecution}to bothQueryEditorHeaderandQueryDebuggerTabsis a smart approach. This maintains consistency across components regarding the run button's enabled or disabled state, enhancing the user experience.Also applies to: 371-371
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11358961297. |
|
Deploy-Preview-URL: https://ce-36893.dp.appsmith.com |
…ppsmithorg#36893) ## Description Updating the permission check for run button in response pane to fix the button being disabled even though the user has execute permissions. Fixes [appsmithorg#36873](appsmithorg#36873) ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11359075861> > Commit: 4bc6d8a > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11359075861&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Wed, 16 Oct 2024 05:26:33 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced the `isRunDisabled` property to enhance the `QueryResponseTab` component, improving execution control based on user permissions. - Updated several components to reflect the new `isRunDisabled` prop, streamlining execution permission handling. - **Bug Fixes** - Renamed `disabled` prop to `isRunDisabled` for clarity and consistency across components. - **Documentation** - Enhanced clarity in the UI regarding execution permissions and action contexts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Updating the permission check for run button in response pane to fix the button being disabled even though the user has execute permissions.
Fixes #36873
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11359075861
Commit: 4bc6d8a
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 16 Oct 2024 05:26:33 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
isRunDisabledproperty to enhance theQueryResponseTabcomponent, improving execution control based on user permissions.isRunDisabledprop, streamlining execution permission handling.Bug Fixes
disabledprop toisRunDisabledfor clarity and consistency across components.Documentation