Hide view and edit tab for account team#672
Conversation
WalkthroughThe pull request introduces comprehensive modifications across multiple service classes within the Goonj CiviCRM extension. The primary focus is on implementing role-based access control for tab visibility, enhancing user permission management, and improving various service methods related to collection camps, dropping centers, and activities. The changes consistently apply similar patterns of role-based checks and method refinements across different service classes, suggesting a systematic approach to access control and functionality enhancement. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)
Line range hint
472-476: Security concern: Sensitive information in URLsThe email URLs contain sensitive information like email addresses and contact numbers in query parameters. This could pose a security risk as:
- URLs might be logged in server logs
- URLs might be visible in browser history
- URLs might be accidentally shared
Consider:
- Moving sensitive data to POST body parameters
- Using encrypted tokens to pass sensitive information
- Storing sensitive data in session and using a session token in URLs
Would you like me to help implement a more secure solution?
Line range hint
366-394: Optimize coordinator assignment logicThe coordinator assignment logic makes multiple database queries that could be optimized:
- Cache the fallback office lookup
- Batch the coordinator queries
- Use a single query with proper joins instead of multiple separate queries
+ private static $fallbackOfficeCache = null; + private static function getFallbackOffice() { + if (self::$fallbackOfficeCache !== null) { + return self::$fallbackOfficeCache; + } + $fallbackOffices = Contact::get(FALSE) ->addSelect('id') ->addWhere('organization_name', 'CONTAINS', self::FALLBACK_OFFICE_NAME) ->execute(); - return $fallbackOffices->first(); + self::$fallbackOfficeCache = $fallbackOffices->first(); + return self::$fallbackOfficeCache; }
🧹 Nitpick comments (12)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionGoonjActivitiesService.php (7)
387-387: Consider using a class constant for restricted roles.Moving the roles array to a class constant would improve maintainability and reusability.
+ const RESTRICTED_ROLES = ['account_team', 'ho_account']; - $restrictedRoles = ['account_team', 'ho_account']; ... - $hasRestrictedRole = !$isAdmin && \CRM_Core_Permission::checkAnyPerm($restrictedRoles); + $hasRestrictedRole = !$isAdmin && \CRM_Core_Permission::checkAnyPerm(self::RESTRICTED_ROLES);Also applies to: 391-391
389-391: Consider combining permission checks.The admin check and restricted role check could be combined into a single method for better reusability.
+ private static function hasRestrictedAccess() { + return !\CRM_Core_Permission::check('admin') && + \CRM_Core_Permission::checkAnyPerm(self::RESTRICTED_ROLES); + } ... - $isAdmin = \CRM_Core_Permission::check('admin'); - $hasRestrictedRole = !$isAdmin && \CRM_Core_Permission::checkAnyPerm($restrictedRoles); + $hasRestrictedRole = self::hasRestrictedAccess();
Line range hint
1-1: Consider adding PHPDoc for the institutionGoonjActivitiesTabset method.The method lacks documentation explaining its purpose and parameters.
+/** + * Manages tab visibility based on user roles for institution Goonj activities. + * + * @param string $tabsetName The name of the tabset + * @param array &$tabs Reference to tabs array + * @param array $context Context information + */ public static function institutionGoonjActivitiesTabset($tabsetName, &$tabs, $context) {Also applies to: 387-397
Line range hint
1-1: Add strict types declaration.Consider adding strict types declaration to enforce type checking.
+declare(strict_types=1); <?php
Line range hint
1-1: Class lacks proper PHPDoc.The class is missing proper documentation explaining its purpose and responsibilities.
/** + * Service class handling Institution Goonj Activities functionality. + * + * This class manages activities, permissions, and notifications for + * Institution Goonj Activities within the CiviCRM system. + */ class InstitutionGoonjActivitiesService extends AutoSubscriber {Also applies to: 18-18
Line range hint
1-1: Consider using typed properties.Class properties should use PHP 7.4+ typed properties for better type safety.
- private static $instituteGoonjActivitiesAddress = NULL; - private static $institutePocAddress = NULL; - private static $addressAdded = FALSE; + private static ?string $instituteGoonjActivitiesAddress = NULL; + private static ?string $institutePocAddress = NULL; + private static bool $addressAdded = FALSE;Also applies to: 20-24
Line range hint
1-1: Consider implementing an interface.The class could benefit from implementing an interface to define its contract.
Consider creating an
InstitutionGoonjActivitiesServiceInterfaceto define the required methods and improve maintainability.wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
93-103: Consider refactoring repeated role-check logic into a shared utility.The lines introduced for removing 'view' and 'edit' tabs if the user has restricted roles are solid. However, this same pattern is repeated across multiple service classes, suggesting duplication. A shared helper or trait to manage these tab restrictions could promote better maintainability.
Example refactor (optional):
-class CollectionCampService extends AutoSubscriber { +abstract class AbstractCampService extends AutoSubscriber { + protected static function hideViewEditTabsForRestrictedRoles(&$tabs) { + $restrictedRoles = ['account_team', 'ho_account']; + $isAdmin = \CRM_Core_Permission::check('admin'); + $hasRestrictedRole = !$isAdmin && \CRM_Core_Permission::checkAnyPerm($restrictedRoles); + if ($hasRestrictedRole) { + unset($tabs['view'], $tabs['edit']); + } + } +} public static function collectionCampTabset($tabsetName, &$tabs, $context) { // ... - $restrictedRoles = ['account_team', 'ho_account']; - $isAdmin = \CRM_Core_Permission::check('admin'); - $hasRestrictedRole = !$isAdmin && \CRM_Core_Permission::checkAnyPerm($restrictedRoles); - if ($hasRestrictedRole) { - unset($tabs['view']); - unset($tabs['edit']); - } + self::hideViewEditTabsForRestrictedRoles($tabs); // ... }wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (1)
560-570: Centralize the restricted role checks.You are consistently checking for restricted roles, repeating the same code snippet. Extracting this logic into a shared helper method or trait could simplify maintenance and ensure consistency.
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (1)
618-628: Streamline the role-based tab hiding across services.The repeated block for verifying admin vs. restricted roles across multiple service classes can be unified. This reduces code duplication and centralizes policy changes.
wp-content/civi-extensions/goonjcustom/Civi/GoonjActivitiesService.php (1)
351-361: Eliminate repeated code for removing tabs.As with other files, relocating the tab removal logic to a single shared location (e.g., a trait or helper class) would improve maintainability and avoid scattered duplication.
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1)
847-857: LGTM! Consider some minor improvements for maintainability.The role-based access control implementation looks good. However, here are some suggestions to make it more maintainable and robust:
+ // Define role constants at the class level + const ROLE_ACCOUNT_TEAM = 'account_team'; + const ROLE_HO_ACCOUNT = 'ho_account'; + public static function institutionCollectionCampTabset($tabsetName, &$tabs, $context) { if (!self::isViewingInstituteCollectionCamp($tabsetName, $context)) { return; } - $restrictedRoles = ['account_team', 'ho_account']; + $restrictedRoles = [self::ROLE_ACCOUNT_TEAM, self::ROLE_HO_ACCOUNT]; $isAdmin = \CRM_Core_Permission::check('admin'); $hasRestrictedRole = !$isAdmin && \CRM_Core_Permission::checkAnyPerm($restrictedRoles); - if ($hasRestrictedRole) { + if ($hasRestrictedRole && isset($tabs['view'], $tabs['edit'])) { unset($tabs['view']); unset($tabs['edit']); }The changes:
- Move role strings to constants for better maintainability
- Add defensive check for tab existence before unsetting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/GoonjActivitiesService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionGoonjActivitiesService.php(1 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionGoonjActivitiesService.php (1)
387-397: LGTM: Role-based access control implementation looks good.
The implementation correctly handles:
- Defining restricted roles
- Checking admin permissions
- Removing view/edit tabs for restricted roles
1 participant
Hide view and edit tab for account team
Summary by CodeRabbit
New Features
Bug Fixes
Refactor