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

Communication: Hide notifications for active channel on lecture and exercise page #7406

Conversation

lennart-keller
Copy link
Contributor

@lennart-keller lennart-keller commented Oct 17, 2023

Checklist

General

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data.
  • I followed the coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.

Motivation and Context

Notifications are shown for the active channel on the lecture/exercise detail pages, even though the channel is open.

Description

Notifications for the open channel are temporarily disabled as long as the sidebar is visible

Steps for Testing

Prerequisites:

  • 2 Students
  • 1 Course with messaging enabled
  1. Go to the same exercise/lecture details page with both students
  2. Write messages as student 1
  3. Student 2 should see the message, but no notification
  4. Go to the messages tab with student 1 and write a message in a different channel (which student 2 is also a member of).
  5. Student 2 should see a notification for this

Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
discussion-section.component.ts 86.77%
notification.service.ts 75%
notification-settings.service.ts 94.44%

Screenshots

No UI changes

@lennart-keller lennart-keller requested a review from a team as a code owner October 17, 2023 12:57
@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Oct 17, 2023
Kroko-fant
Kroko-fant previously approved these changes Oct 17, 2023
Copy link
Contributor

@Kroko-fant Kroko-fant left a comment

Choose a reason for hiding this comment

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

Tested in a testing session worked as expected

Copy link
Contributor

@basak-akan basak-akan left a comment

Choose a reason for hiding this comment

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

Tested in the testing session on TS2, notifications for the open channel disabled 👍

rstief
rstief previously approved these changes Oct 17, 2023
Copy link
Contributor

@rstief rstief left a comment

Choose a reason for hiding this comment

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

Tested in session on ts2

Copy link
Collaborator

@MaximilianAnzinger MaximilianAnzinger left a comment

Choose a reason for hiding this comment

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

Code lgtm

basak-akan
basak-akan previously approved these changes Oct 17, 2023
Copy link
Contributor

@basak-akan basak-akan left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

@@ -91,6 +93,7 @@ export class DiscussionSectionComponent extends CourseDiscussionDirective implem
* on leaving the page, the modal should be closed
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is not up to date now

Copy link
Contributor

@jakubriegel jakubriegel left a comment

Choose a reason for hiding this comment

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

Small note about one comment

@krusche krusche modified the milestones: 6.6.1, 6.6.2 Oct 17, 2023
Copy link
Member

@krusche krusche left a comment

Choose a reason for hiding this comment

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

can you write some client tests to verify the new behavior

@krusche krusche added this to the 6.6.4 milestone Oct 28, 2023
Copy link

github-actions bot commented Nov 5, 2023

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 5, 2023
@krusche krusche modified the milestones: 6.6.4, 6.6.5 Nov 5, 2023
@github-actions github-actions bot removed the stale label Nov 6, 2023
@krusche krusche modified the milestones: 6.6.5, 6.6.6, 6.6.7, 6.7.0 Nov 11, 2023
Copy link

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 24, 2023
@krusche krusche modified the milestones: 6.7.0, 6.7.1 Nov 25, 2023
@github-actions github-actions bot removed the stale label Nov 26, 2023
Copy link

github-actions bot commented Dec 4, 2023

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

@github-actions github-actions bot added the stale label Dec 4, 2023
@krusche krusche modified the milestones: 6.7.1, 6.7.2 Dec 4, 2023
@github-actions github-actions bot removed the stale label Dec 5, 2023
@krusche krusche modified the milestones: 6.7.2, 6.7.3 Dec 8, 2023
@lennart-keller lennart-keller deleted the bugfix/communication/disable-notifications-on-active-channel branch December 14, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants