-
Notifications
You must be signed in to change notification settings - Fork 305
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
: Rename 'hidden' to 'archived' for conversations
#10385
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughThe changes modify the channel management functionality by renaming properties and constants from "hiddenChannels" to "archivedChannels" across the codebase. In addition, conditional logic has been updated in the conversation messaging component to check for changes in the archived status. The changes also update UI elements, localization labels, and associated tests to align with the new terminology. Changes
Sequence Diagram(s)sequenceDiagram
participant C as ConversationMessagesComponent
participant S as Conversation Service
participant M as Model
C->>S: subscribeToActiveConversation()
S->>C: Emit new conversation event
alt Archived status changed
C->>M: Update _activeConversation with new conversation
else
C-->>M: No update
end
sequenceDiagram
participant U as User
participant CO as ConversationOptionsComponent
participant S as Conversation Service
U->>CO: Click on Archive button
CO->>CO: onArchiveClicked(event) – stop propagation, validate IDs
CO->>S: Trigger archive update for conversation
S-->>CO: Return updated conversation status
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/webapp/i18n/en/student-dashboard.json (1)
73-73
: Duplicate or Redundant Key Check: "archived"
There is a key"archived": "Archived"
following the updated key. Please verify whether this key is needed for a different UI context or if it might be redundant. Consolidating similar keys can help maintain clarity and avoid potential inconsistencies in localization.src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (2)
188-190
: Improve efficiency and readability of the archived status check.The condition can be made more efficient and readable by storing the channel DTOs and using descriptive variable names.
- if (this._activeConversation && getAsChannelDTO(conversation)?.isArchived !== getAsChannelDTO(this._activeConversation)?.isArchived) { + const newChannel = getAsChannelDTO(conversation); + const currentChannel = getAsChannelDTO(this._activeConversation); + if (this._activeConversation && newChannel?.isArchived !== currentChannel?.isArchived) { this._activeConversation = conversation; }
131-132
: Align variable names with "archived" terminology.For consistency with the PR's objective of renaming "hidden" to "archived", consider renaming these variables.
- isHiddenInputWithCallToAction = false; - isHiddenInputFull = false; + isArchivedInputWithCallToAction = false; + isArchivedInputFull = false;Remember to update all references to these variables in the template file as well.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
(5 hunks)src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
(1 hunks)src/main/webapp/app/overview/course-overview.service.ts
(2 hunks)src/main/webapp/app/shared/sidebar/conversation-options/conversation-options.component.html
(1 hunks)src/main/webapp/app/shared/sidebar/conversation-options/conversation-options.component.ts
(3 hunks)src/main/webapp/app/types/sidebar.ts
(1 hunks)src/main/webapp/i18n/de/conversation.json
(1 hunks)src/main/webapp/i18n/de/student-dashboard.json
(1 hunks)src/main/webapp/i18n/en/conversation.json
(1 hunks)src/main/webapp/i18n/en/student-dashboard.json
(1 hunks)src/test/javascript/spec/component/course/course-overview.service.spec.ts
(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/i18n/de/student-dashboard.json
🧰 Additional context used
📓 Path-based instructions (4)
`src/main/webapp/i18n/de/**/*.json`: German language transla...
src/main/webapp/i18n/de/**/*.json
: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/conversation.json
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
src/main/webapp/app/types/sidebar.ts
src/main/webapp/app/overview/course-overview.service.ts
src/main/webapp/app/shared/sidebar/conversation-options/conversation-options.component.ts
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html
: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/shared/sidebar/conversation-options/conversation-options.component.html
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts
: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/course/course-overview.service.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (16)
src/main/webapp/app/types/sidebar.ts (1)
28-28
: LGTM! Type definition updated correctly.The change from 'hiddenChannels' to 'archivedChannels' in the ChannelGroupCategory type is consistent with the PR objectives to improve terminology clarity.
src/main/webapp/app/shared/sidebar/conversation-options/conversation-options.component.ts (3)
5-5
: LGTM! Icon imports updated appropriately.The change from eye icons to box archive icons better represents the archiving functionality.
61-62
: LGTM! Icon properties updated consistently.The new icon properties align with the updated imports and better represent the archive/unarchive actions.
82-97
: LGTM! Method renamed while preserving functionality.The method rename from
onHiddenClicked
toonArchiveClicked
maintains the same functionality while using clearer terminology.src/main/webapp/app/overview/course-overview.service.ts (2)
68-68
: LGTM! Default channel groups updated correctly.The change from 'hiddenChannels' to 'archivedChannels' in DEFAULT_CHANNEL_GROUPS is consistent with the type changes.
177-177
: LGTM! Channel grouping logic updated appropriately.The getConversationGroup method now correctly uses 'archivedChannels' instead of 'hiddenChannels'.
src/test/javascript/spec/component/course/course-overview.service.spec.ts (1)
456-456
: LGTM! Test cases updated to reflect new terminology.The test cases have been correctly updated to use "archived" terminology while maintaining the same test coverage and assertions.
Also applies to: 469-469, 479-479
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (4)
7-7
: LGTM!The import of
faBoxArchive
icon is appropriate for the new archiving functionality.
70-70
: LGTM!The renaming from
hiddenChannels
toarchivedChannels
inDEFAULT_CHANNEL_GROUPS
improves clarity of the feature's purpose.
82-82
: LGTM!The icon change to
faBoxArchive
inCHANNEL_TYPE_ICON
provides better visual representation of the archiving action.
96-96
: LGTM!The consistent renaming in
DEFAULT_COLLAPSE_STATE
andDEFAULT_SHOW_ALWAYS
maintains the new terminology across the component.Also applies to: 110-110
src/main/webapp/app/shared/sidebar/conversation-options/conversation-options.component.html (1)
29-31
: LGTM!The changes to the button improve UX by:
- Using more accurate method name
onArchiveClicked
- Using appropriate archive-related icons that better represent the action
src/main/webapp/i18n/en/conversation.json (1)
57-58
: LGTM!The label changes from "show"/"hide" to "Unarchive"/"Archive" better reflect the actual functionality and improve user understanding.
src/main/webapp/i18n/de/conversation.json (1)
39-40
: LGTM!The German translations "Dearchivieren"/"Archivieren" correctly match the English terms and maintain informal language (dutzen) as required.
src/main/webapp/i18n/en/student-dashboard.json (1)
69-69
: Key Rename Confirmation: "archivedChannels"
The key"archivedChannels": "Archived"
correctly replaces the old"hiddenChannels"
key, and the new translation value reflects the intended renaming for clarity in the UI.src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
186-198
:❓ Verification inconclusive
Verify the component's behavior with archived conversations.
The new condition only updates
_activeConversation
when the archived status changes. Ensure this doesn't prevent other important state changes from being reflected in the UI.
🏁 Script executed:
#!/bin/bash # Search for other properties that might need to trigger an update ast-grep --pattern 'getAsChannelDTO($_).$_'Length of output: 169
Action Required: Double-check archived conversation update logic
Please verify that limiting updates of
_activeConversation
to changes in the archived status truly covers all necessary state changes for the component. In particular, ensure that if other conversation properties (such as title, participants, etc.) change while the archived flag remains the same, these updates are either handled elsewhere or intentionally ignored. It might be useful to run additional searches—e.g., using a filtered search such as:#!/bin/bash # Search for getAsChannelDTO usage in TypeScript files, excluding test files rg "getAsChannelDTO" -g '!src/test/**' --type ts—to confirm that no other crucial properties are expected to trigger a UI update. Manual verification or targeted tests in this scenario is recommended.
@@ -36,8 +36,8 @@ | |||
}, | |||
"conversationSelectionSideBar": { | |||
"header": "Konversationen", | |||
"show": "Konversationen zeigen", | |||
"hide": "Konversationen ausblenden", | |||
"show": "Dearchivieren", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds a bit odd to me, i think something along "Wiederherstellen" might be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM ✅ Left an optional comment.
Checklist
General
Client
Motivation and Context
If a channel is currently open and gets archived for everyone, the channel should become read-only immediately, and the input area should become invisible. Currently, the input area remains visible until a refresh, leading to error messages. (Closes
Communication
: Input area remains active for archived channels without refreshing #9986)As decided in the communication subgroup, the "Hide" channel feature should be renamed to "Archive" to avoid confusion:
Description
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
Client
Screenshots
archived section

Summary by CodeRabbit
New Features
Refactor