Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Admin announcement feature #7067

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ahmadhamzh
Copy link
Contributor

@ahmadhamzh ahmadhamzh commented Jan 8, 2025

What this PR does / why we need it:
Add a new feature to the admin page, allowing admins to broadcast announcements to users. Users will see the most recent announcement displayed as a banner across all pages and can view a list of all announcements by opening the announcement dialog from the help panel menu.

Admin- Announcement Page:
image

**add/edit announcement dialog: **
image

Announcement banner:
image

image

list of announcements dialog:
image

Which issue(s) this PR fixes:

Fixes #6645

What type of PR is this?
/kind feature

Does this PR introduce a user-facing change? Then add your Release Note here:

Added a new admin announcement feature. Admins can broadcast messages to users, displayed as banners on all pages, with a full list accessible through the help panel.

Documentation:

TBD

@kubermatic-bot kubermatic-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/docs-needed Indicates that a PR should not merge because it's missing one of the documentation labels. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. sig/api Denotes a PR or issue as being assigned to SIG API. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 8, 2025
@ahmadhamzh ahmadhamzh force-pushed the 6645-admin-announcement-feature branch from c497d5b to 0fdb818 Compare January 8, 2025 12:38
@kubermatic-bot kubermatic-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. docs/tbd Denotes a PR that needs documentation (change) that will be done later. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. do-not-merge/docs-needed Indicates that a PR should not merge because it's missing one of the documentation labels. labels Jan 9, 2025
@ahmadhamzh
Copy link
Contributor Author

/retest

@ahmadhamzh ahmadhamzh force-pushed the 6645-admin-announcement-feature branch from 17ca387 to 1b38c1b Compare January 9, 2025 14:13
@ahmadhamzh ahmadhamzh changed the title WIP Admin announcement feature Admin announcement feature Jan 9, 2025
@kubermatic-bot kubermatic-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 9, 2025
@csengerszabo
Copy link
Contributor

Expires date and Expires time should be Expire date and Expire time

+Add in the button of the add/edit announcement dialog should be Save

Otherwise, looking good!

@ahmadhamzh
Copy link
Contributor Author

+Add in the button of the add/edit announcement dialog should be Save

@csengerszabo
I use "Add" for new announcements, but when the dialog is for editing, it changes to "Save."
The picture above shows adding a new announcement after filling in the fields.

@ahmadhamzh ahmadhamzh force-pushed the 6645-admin-announcement-feature branch from 1b38c1b to be40502 Compare January 10, 2025 08:50
@Waseem826 Waseem826 self-requested a review January 10, 2025 15:01
@Waseem826 Waseem826 added the needs-release-testing Denotes a PR or issue that needs to be tested before release. label Jan 10, 2025
@ahmadhamzh
Copy link
Contributor Author

/retest

Copy link
Contributor

@Waseem826 Waseem826 left a comment

Choose a reason for hiding this comment

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

I've left some feedback after reviewing the UI part. PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert changes in this file. If this needs to be updated then it should be done in separate PR.

if (data) {
const adminAnnouncementsID = Object.keys(announcementsObject);
const readAnnouncements = data.filter(
(value, index, self) => self.indexOf(value) === index && adminAnnouncementsID.includes(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious why do we need self.indexOf(value) === index? Isn't this always true? Also noticed this in announcement-banner component.

class="km-option-hover-bg"
id="km-navbar-api-docs-btn"
matRipple
(click)="goToAPIDocs()">
API Documentation
</div>

<div *ngFor="let link of settings?.customLinks | kmLinkLocation: 'HelpPanel'"
<div *ngIf="adminSettings?.displayAPIDocs"
Copy link
Contributor

Choose a reason for hiding this comment

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

This check should be removed.

Suggested change
<div *ngIf="adminSettings?.displayAPIDocs"
<div *ngIf="adminSettings?.displayAPIDocs"

class="km-option-hover-bg"
matRipple
(click)="openAnnouncementsDialog()">
Announcement
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit pick

Suggested change
Announcement
Announcements

@@ -0,0 +1,163 @@
// Copyright 2024 The Kubermatic Kubernetes Platform contributors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update other newly added files as well.

Suggested change
// Copyright 2024 The Kubermatic Kubernetes Platform contributors.
// Copyright 2025 The Kubermatic Kubernetes Platform contributors.

limitations under the License.
-->

<km-dialog-title>Announcement</km-dialog-title>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<km-dialog-title>Announcement</km-dialog-title>
<km-dialog-title>Announcements</km-dialog-title>

class="km-mat-row"></tr>
</table>
<div class="km-row km-empty-list-msg"
*ngIf="hasAnnouncements()">No announcements available.</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should will not be displayed if we don't have announcements which is the exact opposite of what we want here.

mat-icon-button
(click)="markAsRead(element)"
fxLayoutAlign="center center">
<i Class="km-icon-mask km-icon-show"></i>
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets change this icon to checkmark ✔️)

return !this.announcements?.size;
}

markAsRead(announcement: string): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of relying on parent component to update user settings, we should update them here. WDYT?

@@ -106,6 +106,7 @@ export enum AdminPanelView {
Defaults = 'defaults',
Limits = 'limits',
Customization = 'customization',
Announcement = 'announcement',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for the changes below.

Suggested change
Announcement = 'announcement',
Announcements = 'announcements',

@ahmedwaleedmalik
Copy link
Member

I'd let @Waseem826 review the PR since he is already on it. Just a side-note, please don't forget to create a testing ticket for this.

@ahmadhamzh ahmadhamzh force-pushed the 6645-admin-announcement-feature branch from be40502 to 3e56a71 Compare January 16, 2025 09:09
@ahmadhamzh ahmadhamzh force-pushed the 6645-admin-announcement-feature branch from 3e56a71 to 7ecfa14 Compare January 16, 2025 09:35
@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from waseem826. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ahmadhamzh ahmadhamzh requested a review from Waseem826 January 16, 2025 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. docs/tbd Denotes a PR that needs documentation (change) that will be done later. kind/feature Categorizes issue or PR as related to a new feature. needs-release-testing Denotes a PR or issue that needs to be tested before release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api Denotes a PR or issue as being assigned to SIG API. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin announcement feature
5 participants