Remove the urban ops monetary contribution tab for admin#485
Remove the urban ops monetary contribution tab for admin#485tarunnjoshi merged 4 commits intodevelopfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new dependency on Changes
Possibly related PRs
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
CodeRabbit Configuration File (
|
tarunnjoshi
changed the title
![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 (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
Line range hint
27-27: Consider splitting this class into smaller, focused service classes.The
CollectionCampServiceclass has grown to handle multiple responsibilities, which violates the Single Responsibility Principle. Consider extracting the following functionalities into separate service classes:
CollectionCampEmailService- For email notification logicCollectionCampQRService- For QR code generationCollectionCampFormService- For form processingCollectionCampStatusService- For status managementCollectionCampLogisticsService- For logistics handlingCollectionCampMonetaryService- For monetary contribution managementThis refactoring would:
- Improve code maintainability
- Make the codebase easier to test
- Reduce the risk of merge conflicts
- Make the code more modular
Would you like me to provide a detailed refactoring plan?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(2 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
23-23: LGTM: Import statement is correctly placed.
The addition of CRM_Core_Permission import is necessary for the new permission check functionality.
| // Check if the user is an admin. | ||
| $isAdmin = CRM_Core_Permission::check('administer CiviCRM'); | ||
|
|
||
| // Skip the "monetaryContributionForUrbanOps" tab for admins. | ||
| if ($key == 'monetaryContributionForUrbanOps' && $isAdmin) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Potential inconsistency in admin permission handling
The monetaryContributionForUrbanOps tab appears in both CollectionCampService.php and DroppingCenterService.php, but the admin visibility restriction is only implemented in CollectionCampService.php. This inconsistency could lead to confusion where admins see the tab in one context but not in another.
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php: Missing admin permission check formonetaryContributionForUrbanOpstabwp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php: Has admin permission check
Consider applying the same visibility rules consistently across both services to maintain a uniform user experience.
🔗 Analysis chain
LGTM: Permission check is correctly implemented.
The implementation effectively prevents admin users from seeing the "monetaryContributionForUrbanOps" tab, which aligns with the PR objective.
Let's verify that this is the only place where monetary contribution for urban ops is referenced:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other references to monetaryContributionForUrbanOps
rg -i "monetaryContributionForUrbanOps"
Length of output: 810
Script:
#!/bin/bash
# Check if similar admin permission check exists in DroppingCenterService.php
ast-grep --pattern 'if ($_ == "monetaryContributionForUrbanOps" && $_) {' wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php
Length of output: 145
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (2)
556-562: Fix indentation and remove redundant commentThe indentation of the added code block doesn't match the surrounding code style, and the comment is redundant as the code is self-explanatory.
- // Check if the user is an admin. - $isAdmin = \CRM_Core_Permission::check('admin'); - - // Skip the "monetaryContributionForUrbanOps" tab for admins. - if ($key == 'monetaryContributionForUrbanOps' && $isAdmin) { - continue; - } + $isAdmin = \CRM_Core_Permission::check('admin'); + if ($key == 'monetaryContributionForUrbanOps' && $isAdmin) { + continue; + }
556-562: Consider refactoring permission checksThe current implementation performs multiple permission checks in the loop (admin check and permission check). Consider refactoring to improve efficiency and maintainability.
public static function droppingCenterTabset($tabsetName, &$tabs, $context) { + $isAdmin = \CRM_Core_Permission::check('admin'); foreach ($tabConfigs as $key => $config) { - $isAdmin = \CRM_Core_Permission::check('admin'); - - if ($key == 'monetaryContributionForUrbanOps' && $isAdmin) { + if ($isAdmin && $key == 'monetaryContributionForUrbanOps') { continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php
| // Check if the user is an admin. | ||
| $isAdmin = \CRM_Core_Permission::check('admin'); | ||
|
|
||
| // Skip the "monetaryContributionForUrbanOps" tab for admins. | ||
| if ($key == 'monetaryContributionForUrbanOps' && $isAdmin) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Extract permission check and tab skipping logic to a shared trait
The permission check and tab skipping logic for admin users is indeed duplicated between DroppingCenterService and CollectionCampService. However, since both services already use shared traits (QrCodeable and CollectionSource), the best approach would be to extract this common functionality into a new trait rather than a utility method.
- Create a new trait
AdminTabManagementto handle the shared logic for managing admin tabs - Both services can then use this trait to avoid code duplication
- The trait should contain the common logic for checking admin permissions and skipping specific tabs
Example implementation:
trait AdminTabManagement {
private static function shouldSkipTabForAdmin($tabKey) {
return $tabKey == 'monetaryContributionForUrbanOps' && \CRM_Core_Permission::check('admin');
}
}🔗 Analysis chain
Extract permission logic to a separate method
The permission check logic appears to be duplicated between DroppingCenterService and CollectionCampService. Consider extracting this into a shared utility method.
Suggested refactor:
+ private static function shouldSkipTabForAdmin($tabKey) {
+ return $tabKey == 'monetaryContributionForUrbanOps' && \CRM_Core_Permission::check('admin');
+ }
public static function droppingCenterTabset($tabsetName, &$tabs, $context) {
foreach ($tabConfigs as $key => $config) {
- $isAdmin = \CRM_Core_Permission::check('admin');
- if ($key == 'monetaryContributionForUrbanOps' && $isAdmin) {
+ if (self::shouldSkipTabForAdmin($key)) {
continue;
}Confirmed: Permission check logic is duplicated
The code for checking admin permissions and skipping the "monetaryContributionForUrbanOps" tab is indeed duplicated between DroppingCenterService and CollectionCampService. Let's verify if these are part of the same inheritance hierarchy or if they share a common interface.
Let me try a different approach to check the class definitions and their relationships.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the duplication of permission check logic
rg -A 5 "CRM_Core_Permission::check\('admin'\)" --type php
Length of output: 1342
Script:
#!/bin/bash
# Check if these services share a common parent or interface
ast-grep --pattern 'class DroppingCenterService extends $_'
ast-grep --pattern 'class CollectionCampService extends $_'
ast-grep --pattern 'class DroppingCenterService implements $_'
ast-grep --pattern 'class CollectionCampService implements $_'
Length of output: 37341
Script:
#!/bin/bash
# Get the content of both service files to analyze their structure
cat wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php
cat wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php
Length of output: 66727
| ]; | ||
|
|
||
| foreach ($tabConfigs as $key => $config) { | ||
| // Check if the user is an admin. |
There was a problem hiding this comment.
We don't need comments for this
| // Check if the user is an admin. | ||
| $isAdmin = CRM_Core_Permission::check('admin'); | ||
|
|
||
| // Skip the "monetaryContributionForUrbanOps" tab for admins. |
| ]; | ||
|
|
||
| foreach ($tabConfigs as $key => $config) { | ||
| // Check if the user is an admin. |
| // Check if the user is an admin. | ||
| $isAdmin = \CRM_Core_Permission::check('admin'); | ||
|
|
||
| // Skip the "monetaryContributionForUrbanOps" tab for admins. |
2 participants
Remove the urban ops monetary contribution for admin
Summary by CodeRabbit
New Features
Bug Fixes
Chores