Skip to content

725 create edit teams permissions#803

Merged
kunal0137 merged 7 commits intodevfrom
725-create_editTeamsPermissions
Jun 4, 2025
Merged

725 create edit teams permissions#803
kunal0137 merged 7 commits intodevfrom
725-create_editTeamsPermissions

Conversation

@Subhadeepghosh1
Copy link
Copy Markdown
Contributor

Description:
Created a function for new endpoint

Changes Made:
AdminSecurityGroupUtils.java

@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

725 create edit teams permissions


User description

Description:
Created a function for new endpoint

Changes Made:
AdminSecurityGroupUtils.java


PR Type

Enhancement


Description

  • Add editGroupDetailsAndPropagate method in AdminSecurityGroupUtils

  • Update SMSS_GROUP with new ID, type, description, flag

  • Propagate updates to permission tables in transaction

  • Handle null types, CLOB descriptions, rollback on errors


Changes walkthrough 📝

Relevant files
Enhancement
AdminSecurityGroupUtils.java
Add edit and propagate group details                                         

src/prerna/auth/utils/AdminSecurityGroupUtils.java

  • Introduce editGroupDetailsAndPropagate utility method
  • Execute update query on SMSS_GROUP table
  • Propagate ID/type changes to permission tables
  • Ensure CLOB handling and transactional rollback
  • +79/-1   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /review

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Null Handling

    The ternary expressions for converting curGroupType and newGroupType to uppercase are confusing and could be simplified to avoid potential null pointer issues and improve readability.

    curGroupType = curGroupType == null ? curGroupType : curGroupType.toUpperCase();
    newGroupType = newGroupType == null ? newGroupType : newGroupType.toUpperCase();
    Primary Key Update

    Updating the primary key ID in SMSS_GROUP and manually propagating it to permission tables may violate referential integrity constraints. Verify cascade settings or adjust the update strategy to maintain data consistency.

    groupQuery = "UPDATE SMSS_GROUP SET ID=?, TYPE=?, DESCRIPTION=?, IS_CUSTOM_GROUP=? WHERE ID=?";
    propagateQueries = new String[] {
    		"UPDATE GROUPENGINEPERMISSION SET ID=?, TYPE=? WHERE ID=?",
    		"UPDATE GROUPPROJECTPERMISSION SET ID=?, TYPE=? WHERE ID=?",
    		"UPDATE GROUPINSIGHTPERMISSION SET ID=?, TYPE=? WHERE ID=?", 
    };

    @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /improve

    @Subhadeepghosh1 Subhadeepghosh1 linked an issue May 12, 2025 that may be closed by this pull request
    5 tasks
    @QodoAI-Agent
    Copy link
    Copy Markdown

    QodoAI-Agent commented May 12, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 26b3bb6

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Enable transaction for updates

    After obtaining the JDBC connection, disable auto-commit to ensure all updates and
    propagations occur within a single transaction and can be rolled back together. This
    prevents partial commits if a later statement fails.

    src/prerna/auth/utils/AdminSecurityGroupUtils.java [369]

     conn = securityDb.makeConnection();
    +conn.setAutoCommit(false);
    Suggestion importance[1-10]: 9

    __

    Why: Disabling auto-commit is essential to ensure all updates and propagations occur within a single transaction and can be rolled back together on failure.

    High
    Always close database connection

    Always close the Connection in the finally block regardless of pooling to avoid
    leaking resources when connection pooling is disabled.

    src/prerna/auth/utils/AdminSecurityGroupUtils.java [417-420]

     finally {
    -    if(securityDb.isConnectionPooling() && conn != null) {
    +    if (conn != null) {
             try {
                 conn.close();
    Suggestion importance[1-10]: 8

    __

    Why: Ensuring conn.close() is called regardless of pooling prevents potential resource leaks when pooling is disabled.

    Medium
    Validate new group ID uniqueness

    Add a check to prevent renaming a group to an existing ID if the new ID differs,
    throwing an exception to avoid primary key conflicts. Place this after the existing
    existence check.

    src/prerna/auth/utils/AdminSecurityGroupUtils.java [354]

    -if(!groupExists(curGroupId, curGroupType)) {
    +if (!groupExists(curGroupId, curGroupType)) {
    +    throw new IllegalArgumentException("Group " + curGroupId + " does not exist");
    +}
    +if (!curGroupId.equals(newGroupId) && groupExists(newGroupId, newGroupType)) {
    +    throw new IllegalArgumentException("Group " + newGroupId + " already exists");
    +}
    Suggestion importance[1-10]: 7

    __

    Why: Adding a check prevents renaming to an existing group ID and avoids primary key conflicts, improving data integrity.

    Medium
    General
    Use locale-safe uppercase conversion

    Use a fixed locale for toUpperCase to avoid unexpected behavior in different
    locales, for example Locale.ENGLISH.

    src/prerna/auth/utils/AdminSecurityGroupUtils.java [351-352]

    -curGroupType = curGroupType == null ? curGroupType : curGroupType.toUpperCase();
    -newGroupType = newGroupType == null ? newGroupType : newGroupType.toUpperCase();
    +curGroupType = curGroupType == null ? null : curGroupType.toUpperCase(Locale.ENGLISH);
    +newGroupType = newGroupType == null ? null : newGroupType.toUpperCase(Locale.ENGLISH);
    Suggestion importance[1-10]: 5

    __

    Why: Specifying Locale.ENGLISH prevents unexpected behavior across different locales and cleans up the null handling logic.

    Low

    Previous suggestions

    Suggestions up to commit 8e85c06
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Disable auto-commit for transactions

    Disable auto‐commit immediately after obtaining the connection so that all updates
    and propagations occur within one transaction and can be properly rolled back on
    error.

    src/prerna/auth/utils/AdminSecurityGroupUtils.java [369]

     conn = securityDb.makeConnection();
    +conn.setAutoCommit(false);
    Suggestion importance[1-10]: 8

    __

    Why: Explicitly setting conn.setAutoCommit(false) ensures all operations execute within a single transaction and allows proper rollback on failure.

    Medium
    Prevent duplicate group ID

    Add a precondition checking that newGroupId is not already present to avoid primary
    key conflicts when renaming an existing group.

    src/prerna/auth/utils/AdminSecurityGroupUtils.java [354-357]

     if(!groupExists(curGroupId, curGroupType)) {
         throw new IllegalArgumentException("Group " + curGroupId + " does not exist");
     }
    +if(groupExists(newGroupId, newGroupType)) {
    +    throw new IllegalArgumentException("Group " + newGroupId + " already exists");
    +}
     String groupQuery = null;
    Suggestion importance[1-10]: 7

    __

    Why: Adding a check for newGroupId avoids potential primary key conflicts when renaming an existing group.

    Medium
    Always close database connection

    Always close the JDBC Connection in a finally block regardless of pooling
    configuration to avoid resource leaks.

    src/prerna/auth/utils/AdminSecurityGroupUtils.java [418-424]

    -if(securityDb.isConnectionPooling() && conn != null) {
    +if(conn != null) {
         try {
             conn.close();
         } catch (SQLException e) {
             classLogger.error(Constants.STACKTRACE, e);
         }
     }
    Suggestion importance[1-10]: 7

    __

    Why: Ensuring conn.close() runs regardless of pooling prevents resource leaks when pooling is disabled.

    Medium
    General
    Remove unused variable

    Remove the unused userDetails variable to eliminate dead code or, if intended for
    permission checks, actually use it before proceeding.

    src/prerna/auth/utils/AdminSecurityGroupUtils.java [371]

    -Pair<String, String> userDetails = User.getPrimaryUserIdAndTypePair(user);
    +// line removed as userDetails is not referenced
    Suggestion importance[1-10]: 4

    __

    Why: The variable userDetails is never used after assignment, so removing it cleans up dead code with minimal impact.

    Low
    Suggestions up to commit 47d5a8e
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Enable explicit transaction control

    Explicitly disable auto-commit on the connection to ensure all updates and
    propagations run in a single transaction, allowing proper rollback on failure. Call
    conn.setAutoCommit(false) right after opening the connection.

    src/prerna/auth/utils/AdminSecurityGroupUtils.java [358]

     conn = securityDb.makeConnection();
    +conn.setAutoCommit(false);
    Suggestion importance[1-10]: 8

    __

    Why: The code opens a connection but never disables auto-commit, so commit/rollback logic inside !conn.getAutoCommit() never executes as intended.

    Medium
    Validate new group ID presence

    Add validation to ensure newGroupId is not null or blank before executing any
    database updates. This prevents accidental updates to an empty or invalid group ID.

    src/prerna/auth/utils/AdminSecurityGroupUtils.java [343-345]

    -if(!groupExists(curGroupId, curGroupType)) {
    +if (newGroupId == null || newGroupId.trim().isEmpty()) {
    +    throw new IllegalArgumentException("New group ID must not be blank");
    +}
    +if (!groupExists(curGroupId, curGroupType)) {
         throw new IllegalArgumentException("Group " + curGroupId + " does not exist");
     }
    Suggestion importance[1-10]: 6

    __

    Why: Adding a null or blank check for newGroupId prevents accidental updates with an invalid identifier before hitting the database.

    Low
    General
    Simplify type normalization

    Simplify and clarify the normalization of curGroupType and newGroupType by using
    straightforward null checks.

    src/prerna/auth/utils/AdminSecurityGroupUtils.java [340-341]

    -curGroupType = curGroupType == null ? curGroupType : curGroupType.toUpperCase();
    -newGroupType = newGroupType == null ? newGroupType : newGroupType.toUpperCase();
    +if (curGroupType != null) {
    +    curGroupType = curGroupType.toUpperCase();
    +}
    +if (newGroupType != null) {
    +    newGroupType = newGroupType.toUpperCase();
    +}
    Suggestion importance[1-10]: 4

    __

    Why: The suggested if checks are functionally equivalent and improve readability, but this is a minor stylistic change.

    Low

    @Subhadeepghosh1
    Copy link
    Copy Markdown
    Contributor Author

    This pr should be merged along with SEMOSS/Monolith#127

    @AAfghahi AAfghahi requested a review from ppatel9703 May 19, 2025 13:40
    @kunal0137
    Copy link
    Copy Markdown
    Collaborator

    how do i test this? what is the FE branch for this and what page do i need to go to test this?

    @anurag91jain
    Copy link
    Copy Markdown
    Contributor

    how do i test this? what is the FE branch for this and what page do i need to go to test this?

    SEMOSS/semoss-ui#1184
    This is being made from teams page. @Subhadeepghosh1 @ishumita attach a screenshot from FE

    @Subhadeepghosh1
    Copy link
    Copy Markdown
    Contributor Author

    image

    @kunal0137 kunal0137 merged commit 377fadc into dev Jun 4, 2025
    3 checks passed
    @kunal0137 kunal0137 deleted the 725-create_editTeamsPermissions branch June 4, 2025 12:55
    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented Jun 4, 2025

    @CodiumAI-Agent /update_changelog

    @QodoAI-Agent
    Copy link
    Copy Markdown

    Changelog updates: 🔄

    2025-06-04 #803

    Added

    • Endpoint and utility to edit teams permissions, propagating group detail updates across permission tables.

    to commit the new content to the CHANGELOG.md file, please type:
    '/update_changelog --pr_update_changelog.push_changelog_changes=true'

    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.

    [TASK] Create editTeamsPermissions reactor

    5 participants