Skip to content

feat: add configurable global UI banner visibility setting - #3435

Closed
lucas4790 wants to merge 1 commit into
SonarSource:masterfrom
lucas4790:feat/configurable-banner-visibility
Closed

feat: add configurable global UI banner visibility setting#3435
lucas4790 wants to merge 1 commit into
SonarSource:masterfrom
lucas4790:feat/configurable-banner-visibility

Conversation

@lucas4790

@lucas4790 lucas4790 commented May 27, 2026

Copy link
Copy Markdown

What problem are you trying to fix?

Administrators have no way to control who sees global update/upgrade notification banners. This adds a configurable property to give admins that control.

What improvement are you trying to make?

Adds a new property sonar.ui.banners.visibility (Administration > General > Look & Feel) that controls who sees global update/upgrade notification banners.

Value Behaviour
ADMINS_ONLY (default) Only users with global Admin permission see banners — preserves existing behaviour
ALL All logged-in users see update/upgrade banners
DISABLED No users see banners

Checklist

  • Motives explained above
  • Unit test provided in CorePropertyDefinitionsTest
  • Follows existing code style in CorePropertyDefinitions.java

Files changed

  • sonar-core/src/main/java/org/sonar/core/config/CorePropertyDefinitions.java — registers the property definition with options and default value
  • server/sonar-webserver-webapi/src/main/java/org/sonar/server/ui/ws/GlobalAction.java — adds the key to DYNAMIC_SETTING_KEYS so it is served via api/navigation/global
  • sonar-core/src/test/java/org/sonar/core/config/CorePropertyDefinitionsTest.java — verifies the property is registered with the correct options and default

Note: The frontend counterpart (reading the setting and gating UpdateNotification) is in a separate PR against the sonarqube-webapp repository.

@lucas4790
lucas4790 force-pushed the feat/configurable-banner-visibility branch from 0e73bcf to 850bd1f Compare May 27, 2026 08:50
.subCategory(CoreProperties.SUBCATEGORY_LOOKNFEEL)
.type(SINGLE_SELECT_LIST)
.options("ALL", "ADMINS_ONLY", "DISABLED")
.defaultValue("ADMINS_ONLY")

@gitar-bot gitar-bot Bot May 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Bug: Default value contradicts PR description

The PR description states that ALL is the default behaviour ("ALL (default) — All logged-in users see update/upgrade banners"), but the code at line 159 sets .defaultValue("ADMINS_ONLY"). The test also asserts ADMINS_ONLY as the default, so the implementation is internally consistent — but one of the two (docs vs code) is wrong.

If the intended default is ALL (preserving current behaviour for existing instances), the code and test need updating. If ADMINS_ONLY is intentional, update the PR description table to avoid confusion during review and for future readers.

If the intent is to preserve current behaviour (all users see banners) and let admins opt in to restricting visibility, change the default to ALL.:

.defaultValue("ALL")

Was this helpful? React with 👍 / 👎

Comment thread sonar-core/src/test/java/org/sonar/core/config/CorePropertyDefinitionsTest.java Outdated
Add a new property sonar.ui.banners.visibility that controls who sees
global update/upgrade notification banners in the UI.

- ALL (default): all logged-in users see banners
- ADMINS_ONLY: only users with global Admin permission see banners
- DISABLED: no users see banners

The property is registered in CorePropertyDefinitions and exposed via
the api/navigation/global endpoint through GlobalAction.

Provides unit test in CorePropertyDefinitionsTest.
@lucas4790

Copy link
Copy Markdown
Author

Both findings from gitar-bot have been addressed in the latest commit:

  1. Default value / PR description mismatch — The PR description has been updated. ADMINS_ONLY is the intended default, which preserves the existing behavior (only admins see banners). The description now lists ADMINS_ONLY (default) first with a note explaining this.

  2. Inconsistent indentation in test — Fixed. Both assertThat lines inside the satisfies lambda now use consistent 8-space indentation.

@lucas4790

Copy link
Copy Markdown
Author

gitar review

@lucas4790

Copy link
Copy Markdown
Author

gitar unblock

@matteo-mara-sonarsource

matteo-mara-sonarsource commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Thanks @lucas4790 for this contribution.
We are on it and its being discussed with the PMs to see if it matches with the direction of the product.

I will keep you posted.

Plus this change cannot move forward on its own without the changes on the webapp side.
So I will need to check that aspect too.

@matteo-mara-sonarsource

Copy link
Copy Markdown
Contributor

This was discussed with PM and it is not a feature we want to introduce in the product.
Thanks in any case for the contribution.

Best regards,
Matteo

@gitar-bot

gitar-bot Bot commented Jun 5, 2026

Copy link
Copy Markdown
Code Review ⚠️ Changes requested 1 resolved / 2 findings

Introduces the sonar.ui.banners.visibility setting to manage UI banner access, but the default value implementation contradicts the stated design goal of defaulting to 'ALL'.

⚠️ Bug: Default value contradicts PR description

📄 sonar-core/src/main/java/org/sonar/core/config/CorePropertyDefinitions.java:159 📄 sonar-core/src/test/java/org/sonar/core/config/CorePropertyDefinitionsTest.java:68

The PR description states that ALL is the default behaviour ("ALL (default) — All logged-in users see update/upgrade banners"), but the code at line 159 sets .defaultValue("ADMINS_ONLY"). The test also asserts ADMINS_ONLY as the default, so the implementation is internally consistent — but one of the two (docs vs code) is wrong.

If the intended default is ALL (preserving current behaviour for existing instances), the code and test need updating. If ADMINS_ONLY is intentional, update the PR description table to avoid confusion during review and for future readers.

If the intent is to preserve current behaviour (all users see banners) and let admins opt in to restricting visibility, change the default to ALL.
.defaultValue("ALL")
✅ 1 resolved
Quality: Test has inconsistent indentation

📄 sonar-core/src/test/java/org/sonar/core/config/CorePropertyDefinitionsTest.java:68-69
Line 68 uses 10-space indentation while line 69 uses 8-space indentation inside the satisfies lambda. This is a minor formatting inconsistency.

🤖 Prompt for agents
Code Review: Introduces the `sonar.ui.banners.visibility` setting to manage UI banner access, but the default value implementation contradicts the stated design goal of defaulting to 'ALL'.

1. ⚠️ Bug: Default value contradicts PR description
   Files: sonar-core/src/main/java/org/sonar/core/config/CorePropertyDefinitions.java:159, sonar-core/src/test/java/org/sonar/core/config/CorePropertyDefinitionsTest.java:68

   The PR description states that `ALL` is the default behaviour ("ALL (default) — All logged-in users see update/upgrade banners"), but the code at line 159 sets `.defaultValue("ADMINS_ONLY")`. The test also asserts `ADMINS_ONLY` as the default, so the implementation is internally consistent — but one of the two (docs vs code) is wrong.
   
   If the intended default is `ALL` (preserving current behaviour for existing instances), the code and test need updating. If `ADMINS_ONLY` is intentional, update the PR description table to avoid confusion during review and for future readers.

   Fix (If the intent is to preserve current behaviour (all users see banners) and let admins opt in to restricting visibility, change the default to ALL.):
   .defaultValue("ALL")

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.
Unblock → Override a blocking verdict and allow merging.

Comment with these commands to change:

Auto-apply Compact Unblock
gitar auto-apply:on         
gitar display:verbose         
gitar unblock         

Was this helpful? React with 👍 / 👎 | Gitar

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.

3 participants