Skip to content

feat: modify user group menu condition#5996

Merged
skdud4659 merged 1 commit intomasterfrom
hotfix-user-group
Jul 9, 2025
Merged

feat: modify user group menu condition#5996
skdud4659 merged 1 commit intomasterfrom
hotfix-user-group

Conversation

@skdud4659
Copy link
Member

Skip Review (optional)

  • Minor changes that don't affect the functionality (e.g. style, chore, ci, test, docs)
  • Previously reviewed in feature branch, further review is not mandatory
  • Self-merge allowed for solo developers or urgent changes

Description (optional)

SSIA

Things to Talk About (optional)

Signed-off-by: NaYeong,Kim <nayeongkim@megazone.com>
@skdud4659 skdud4659 requested a review from Copilot July 9, 2025 01:19
@vercel
Copy link

vercel bot commented Jul 9, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
console ⬜️ Ignored (Inspect) Jul 9, 2025 1:19am
web-storybook ⬜️ Ignored (Inspect) Jul 9, 2025 1:19am

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2025

✅ There are no commits in this PR that require review.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2025

🎉 @seungyeoneeee has been randomly selected as the reviewer! Please review. 🙏

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refines the condition for inserting the USER_GROUP menu by ensuring the Alert Manager feature is explicitly enabled.

  • Introduces an isEnabledAlertManager flag from config
  • Updates the if check to require both ENABLED and VERSION === 'V2' before adding USER_GROUP
Comments suppressed due to low confidence (3)

apps/web/src/services/iam/configurator.ts:37

  • [nitpick] Consider renaming isEnabledAlertManager to alertManagerEnabled for improved readability and alignment with common boolean naming conventions.
        const isEnabledAlertManager = config?.ALERT_MANAGER?.ENABLED;

apps/web/src/services/iam/configurator.ts:57

  • Add a unit test for the scenario where ALERT_MANAGER.ENABLED is false but VERSION is 'V2' to verify that the USER_GROUP menu is not inserted.
        if (isEnabledAlertManager && alertManagerVersion === 'V2') {

apps/web/src/services/iam/configurator.ts:37

  • [nitpick] For clearer grouping of related properties, consider destructuring ALERT_MANAGER from config (e.g., const { ENABLED: alertManagerEnabled, VERSION: alertManagerVersion } = config?.ALERT_MANAGER ?? {};).
        const isEnabledAlertManager = config?.ALERT_MANAGER?.ENABLED;

Copy link
Contributor

@seungyeoneeee seungyeoneeee left a comment

Choose a reason for hiding this comment

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

LGTM !!

@skdud4659 skdud4659 merged commit 2d59004 into master Jul 9, 2025
13 checks passed
@skdud4659 skdud4659 deleted the hotfix-user-group branch July 9, 2025 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants