[ECO-5196] Message edit/delete #100
Conversation
WalkthroughThis pull request refines chat message handling by updating the ChatApi logic, enhancing message construction, and improving error management. New methods to update and delete messages have been added, and the Message data class now includes versioning, timestamp, and operational details. The EventTypes definitions have been updated for consistent naming, and the Messages interface now supports update and delete operations. Several tests and integration suites have been revised or added to validate these changes, while metadata handling has been refined and the ably library version has been upgraded. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as ChatApi
participant S as Server
C->>API: updateMessage(message, params)
API->>S: Send update request (JSON with version, timestamp, operation)
S-->>API: Updated message details
API->>C: Return updated Message
sequenceDiagram
participant C as Client
participant API as ChatApi
participant S as Server
C->>API: deleteMessage(message, params)
API->>S: Send delete request (JSON with version, timestamp, operation)
S-->>API: Deleted message details
API->>C: Return deleted Message
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
- Updated usages across roomreactions, messages and chatAPI to use JsonObject
sacOO7
force-pushed
the
feature/message-edit-update
branch
from
2883beb to
8cf8d51
Compare
Code Coverage
|
1. Added separate typealias for OperationMetadata 2. Defined messages operation param data classes, UpdateMessageParams and DeleteMessageParams
1. Updated Messages interface, added methods for message update and delete
sacOO7
force-pushed
the
feature/message-edit-update
branch
from
c9c1c98 to
d78946f
Compare
1. Added new fields version, timestamp and operation 2. Added method to decode JsonObject into Message.Operation 3. Updated/fixed tests accordingly
sacOO7
force-pushed
the
feature/message-edit-update
branch
from
bfade60 to
d36ac65
Compare
- Added impl for updateMessage anddeleteMessage - Added missing impl. for message update and delete - Refactored relevant code to have support for message update and delete actions
… tests - Added map for messageActionToEventType, used the same for emitting related message create, update and delete events - Refactored SandboxTest.kt, moved all of tests under `integration` package.
sacOO7
force-pushed
the
feature/message-edit-update
branch
from
8db641b to
44cd11b
Compare
sacOO7
force-pushed
the
feature/message-edit-update
branch
from
44cd11b to
a5bb0fc
Compare
sacOO7
force-pushed
the
feature/message-edit-update
branch
from
a5bb0fc to
5abc0bb
Compare
- Renamed message edit/delete description param to opDescription - Updated response fields for send and update message - Refactored tests for sending and retriving message history - Added test for updating a sent message - Added test for deleting a sent message
sacOO7
force-pushed
the
feature/message-edit-update
branch
from
5abc0bb to
b20593e
Compare
1. Refactored spec annotations for messages according to spec 2. Removed unnecessary send param validation not needed as per spec
sacOO7
force-pushed
the
feature/message-edit-update
branch
from
86acfa6 to
97e05c1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
chat-android/src/test/java/com/ably/chat/integration/ConnectionIntegrationTest.kt (2)
13-29: Consider enhancing test coverage and robustness.While the basic connection flow is tested, consider these improvements:
- Add timeout to prevent test hanging if connection never succeeds
- Add verification of initial Connecting status
- Add cleanup in an
@Aftermethod- Add test cases for connection failures
Here's a suggested implementation:
+ private lateinit var chatClient: ChatClient + @Test fun `should observe connection status`() = runTest { - val chatClient = sandbox.createSandboxChatClient() + chatClient = sandbox.createSandboxChatClient() + assertEquals(ConnectionStatus.Connecting, chatClient.connection.status) val connectionStatusChange = CompletableDeferred<ConnectionStatusChange>() chatClient.connection.onStatusChange { if (it.current == ConnectionStatus.Connected) connectionStatusChange.complete(it) } assertEquals( ConnectionStatusChange( current = ConnectionStatus.Connected, previous = ConnectionStatus.Connecting, error = null, retryIn = 0, ), - connectionStatusChange.await(), + withTimeout(5000) { connectionStatusChange.await() }, ) } + + @After + fun tearDown() { + if (::chatClient.isInitialized) { + runBlocking { chatClient.close() } + } + } + + @Test + fun `should handle connection failure`() = runTest { + // Add test for connection failure scenario + }
31-39: Add sandbox cleanup and error handling.Consider adding cleanup and error handling to prevent resource leaks and improve test reliability.
Here's a suggested implementation:
companion object { private lateinit var sandbox: Sandbox @JvmStatic @BeforeClass fun setUp() = runTest { - sandbox = Sandbox.createInstance() + try { + sandbox = Sandbox.createInstance() + } catch (e: Exception) { + throw IllegalStateException("Failed to create sandbox instance", e) + } } + + @JvmStatic + @AfterClass + fun tearDown() = runTest { + if (::sandbox.isInitialized) { + sandbox.cleanup() + } + } }chat-android/src/test/java/com/ably/chat/integration/RoomIntegrationTest.kt (2)
18-71: Consider adding error case validations.The helper methods thoroughly validate successful state transitions. However, consider adding test cases for:
- Network failures during state transitions
- Concurrent state change operations
- Invalid state transitions
73-95: Consider using deterministic room IDs for tests.Using
UUID.randomUUID()for room IDs could make tests non-deterministic and harder to debug. Consider using fixed test IDs or a deterministic sequence.Example approach:
-val room1 = chatClient.rooms.get(UUID.randomUUID().toString()) +val room1 = chatClient.rooms.get("test-room-1") -val room2 = chatClient.rooms.get(UUID.randomUUID().toString()) +val room2 = chatClient.rooms.get("test-room-2")chat-android/src/test/java/com/ably/chat/integration/TypingIntegrationTest.kt (2)
15-34: Consider enhancing test coverage and resource cleanup.While the basic typing indication test is well-implemented, consider the following improvements:
- Add cleanup in a
@Afterblock to release resources.- Add test cases for typing timeout and explicit stop scenarios.
- Extract the timeout value to a named constant for better maintainability.
Here's a suggested implementation:
+ private companion object { + private const val TYPING_TIMEOUT_MS = 10_000 + } @Test fun `should return typing indication for client`() = runTest { val chatClient1 = sandbox.createSandboxChatClient("client1") val chatClient2 = sandbox.createSandboxChatClient("client2") val roomId = UUID.randomUUID().toString() - val roomOptions = RoomOptions(typing = TypingOptions(timeoutMs = 10_000)) + val roomOptions = RoomOptions(typing = TypingOptions(timeoutMs = TYPING_TIMEOUT_MS)) val chatClient1Room = chatClient1.rooms.get(roomId, roomOptions) chatClient1Room.attach() val chatClient2Room = chatClient2.rooms.get(roomId, roomOptions) chatClient2Room.attach() val deferredValue = CompletableDeferred<TypingEvent>() chatClient2Room.typing.subscribe { deferredValue.complete(it) } chatClient1Room.typing.start() val typingEvent = deferredValue.await() assertEquals(setOf("client1"), typingEvent.currentlyTyping) assertEquals(setOf("client1"), chatClient2Room.typing.get()) + + // Cleanup + chatClient1Room.detach() + chatClient2Room.detach() } + @Test + fun `should handle typing timeout`() = runTest { + // Similar setup... + chatClient1Room.typing.start() + delay(TYPING_TIMEOUT_MS + 100) + assertTrue(chatClient2Room.typing.get().isEmpty()) + } + + @Test + fun `should handle explicit typing stop`() = runTest { + // Similar setup... + chatClient1Room.typing.start() + chatClient1Room.typing.stop() + assertTrue(chatClient2Room.typing.get().isEmpty()) + }
36-44: Consider adding cleanup and error handling for sandbox.The setup is good, but consider adding:
- An
@AfterClassmethod to clean up the sandbox instance.- Error handling for sandbox creation.
Here's a suggested implementation:
companion object { private lateinit var sandbox: Sandbox @JvmStatic @BeforeClass fun setUp() = runTest { - sandbox = Sandbox.createInstance() + try { + sandbox = Sandbox.createInstance() + } catch (e: Exception) { + throw IllegalStateException("Failed to create sandbox instance", e) + } } + + @JvmStatic + @AfterClass + fun tearDown() = runTest { + if (::sandbox.isInitialized) { + sandbox.cleanup() + } + } }chat-android/src/test/java/com/ably/chat/integration/ReactionsIntegrationTest.kt (1)
16-34: Add test coverage for error cases and edge scenarios.While the happy path is covered, consider adding tests for:
- Reaction to non-existent messages
- Multiple reactions
- Invalid reaction types
- Room detachment during reaction
chat-android/src/test/java/com/ably/chat/integration/OccupancyIntegrationTest.kt (1)
17-31: Add test coverage for multi-client scenarios.Consider adding tests for:
- Multiple clients joining/leaving
- Concurrent occupancy changes
- Client disconnection scenarios
chat-android/src/test/java/com/ably/chat/integration/PresenceIntegrationTest.kt (1)
22-30: Add test coverage for presence updates and data.While basic presence is tested, consider adding tests for:
- Presence updates with data
- Multiple clients entering/leaving
- Error cases (e.g., entering twice)
- Connection loss scenarios
chat-android/src/main/java/com/ably/chat/Message.kt (1)
100-119: Consider adding null safety for JSON property access.The JSON property access could be made safer using
getAsString()with null checks.- operation.clientId = jsonObject.get("clientId").asString + jsonObject.get("clientId")?.let { + operation.clientId = it.getAsString() + }chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1)
252-252: Consider adding timestamp to buildDummyPubSubMessage.While version is added to the dummy message, timestamp should also be included for consistency.
version = "abcdefghij@1672531200000-123" +timestamp = 1000Lchat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt (1)
65-88: Reduce code duplication in assertions.The message field assertions are duplicated across tests. Consider extracting these into a helper method to improve maintainability.
+private fun assertMessageFields(expected: Message, actual: Message) { + assertEquals(expected.serial, actual.serial) + assertEquals(expected.clientId, actual.clientId) + assertEquals(expected.roomId, actual.roomId) + assertEquals(expected.text, actual.text) + assertEquals(expected.createdAt, actual.createdAt) + assertEquals(expected.metadata.toString(), actual.metadata.toString()) + assertEquals(expected.headers, actual.headers) + assertEquals(expected.action, actual.action) + assertEquals(expected.version, actual.version) + assertEquals(expected.timestamp, actual.timestamp) + assertEquals(expected.operation, actual.operation) +}Then use it in your tests:
assertMessageFields(sentMessage, receivedMessage)Also applies to: 116-139
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
chat-android/src/main/java/com/ably/chat/ChatApi.kt(3 hunks)chat-android/src/main/java/com/ably/chat/EventTypes.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(9 hunks)chat-android/src/main/java/com/ably/chat/Metadata.kt(2 hunks)chat-android/src/main/java/com/ably/chat/OperationMetadata.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(0 hunks)chat-android/src/main/java/com/ably/chat/RoomReactions.kt(1 hunks)chat-android/src/test/java/com/ably/chat/ChatApiTest.kt(3 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(4 hunks)chat-android/src/test/java/com/ably/chat/SandboxTest.kt(0 hunks)chat-android/src/test/java/com/ably/chat/integration/ConnectionIntegrationTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/integration/OccupancyIntegrationTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/integration/PresenceIntegrationTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/integration/ReactionsIntegrationTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/integration/RoomIntegrationTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/integration/Sandbox.kt(1 hunks)chat-android/src/test/java/com/ably/chat/integration/TypingIntegrationTest.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(1 hunks)gradle/libs.versions.toml(1 hunks)
💤 Files with no reviewable changes (2)
- chat-android/src/main/java/com/ably/chat/Room.kt
- chat-android/src/test/java/com/ably/chat/SandboxTest.kt
✅ Files skipped from review due to trivial changes (2)
- gradle/libs.versions.toml
- chat-android/src/test/java/com/ably/chat/integration/Sandbox.kt
🧰 Additional context used
🧠 Learnings (4)
chat-android/src/test/java/com/ably/chat/integration/ReactionsIntegrationTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
chat-android/src/test/java/com/ably/chat/integration/OccupancyIntegrationTest.kt (2)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/OccupancyTest.kt:20-31
Timestamp: 2024-11-28T11:12:06.843Z
Learning: In `OccupancyTest.kt`, additional room state initialization using `setState` is unnecessary for testing occupancy functionality, as the specification doesn't require it.
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Occupancy.kt:145-145
Timestamp: 2024-11-28T11:11:20.423Z
Learning: In `chat-android/src/main/java/com/ably/chat/Occupancy.kt`, within the `DefaultOccupancy` class, when methods use `room.chatApi`, which utilizes the REST API, there's no need to call `room.ensureAttached()` before performing the operation.
chat-android/src/test/java/com/ably/chat/integration/PresenceIntegrationTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/PresenceTest.kt:33-33
Timestamp: 2024-11-28T11:08:38.559Z
Learning: The `RoomEnsureAttachedTest.kt` file contains tests that verify room operations are only performed when the room is in the `ATTACHED` state, including scenarios where operations fail when the room is not attached, succeed when it is attached, and proper error handling for invalid room states.
chat-android/src/test/java/com/ably/chat/integration/RoomIntegrationTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
🔇 Additional comments (35)
chat-android/src/test/java/com/ably/chat/integration/ConnectionIntegrationTest.kt (1)
1-10: LGTM! Well-organized imports.The imports are clean and appropriate for the test requirements.
chat-android/src/test/java/com/ably/chat/integration/RoomIntegrationTest.kt (2)
1-1: LGTM! Package organization aligns with test purpose.The move to the
integrationpackage better reflects the integration test nature of this class, and the new imports support the lifecycle management functionality.Also applies to: 8-9
97-105: Verify test isolation with shared sandbox.Moving
sandboxto a companion object with@BeforeClassmeans it's shared across all test methods. This could lead to state leakage between tests. Consider:
- Using
@Beforeinstead to create a fresh sandbox for each test- Adding explicit cleanup in
@Afteror@AfterClass- Documenting why sharing the sandbox is safe
chat-android/src/test/java/com/ably/chat/integration/TypingIntegrationTest.kt (1)
1-13: LGTM! Well-organized imports and clear class structure.The imports are specific and necessary for the test implementation, showing good organization.
chat-android/src/main/java/com/ably/chat/ChatApi.kt (5)
38-53: Consider null handling for the "operation" field.Using
getAsJsonObject("operation")may throw if the field is absent or not a valid JSON object. If partial data is possible, consider a safer approach or clarify via documentation that the field's absence is invalid.
60-86: Initialization of version and timestamp looks consistent.Assigning
version = serialandtimestamp = createdAtfor newly sent messages aligns with the system’s creation flow. No further issues identified here.
89-117: Revisit settingclientId = clientIdwhen updating the message.While ensuring the local client is the one performing the update, overwriting the original message’s clientId might risk confusion if the updating user differs from the original sender. Confirm that this behavior is desired.
119-147: Delete flow is coherent and consistent.Soft-delete semantics, building a new Message with
MESSAGE_DELETEaction, versioning, and timestamp are logically sound. No additional concerns.
158-158: Empty response error handling is consistent with other endpoints.Throwing a
serverErroron an empty occupancy response matches prior patterns for consistency.chat-android/src/main/java/com/ably/chat/Messages.kt (15)
63-63: Documentation annotation looks fine.
No material change or issue identified.
73-88: Well-documented update method.
The explanatory comment clarifies usage and behavior thoroughly.
89-97: Method signature forupdateis consistent with the new API.
No issues found.
98-117: Clear and concise delete method documentation.
Describes soft delete behavior thoroughly.
222-228:SendMessageParams.toJsonObject()is straightforward and readable.
Implementation follows established patterns.
230-255:UpdateMessageParamsand conversion to JSON appear correct.
Captures text, headers, and optional description/metadata accurately.
259-275:DeleteMessageParamslogic is clear.
Allows flexible fields for the delete action.
367-368: Event mapping logic is clear.
Throws an error for unknown actions, preventing silent failures.
378-383: Inclusion of new fields inMessageis consistent.
version,timestamp, andoperationalign with the rest of the update/delete flow.
387-387: Subscription withCHAT_MESSAGEis aligned with new naming.
No observed hazards.
399-399: Unsubscription logic updated to match new event name.
No issues identified.
413-418: Send method delegates toChatApiappropriately.
Implementation is concise and consistent.
419-426: Validation: Ensure overwrittenclientIdis correct.As in
ChatApi.updateMessage, confirm that referencing the localclientIdis intended rather than preserving the original author’s.
435-442: Delete method usage matches the ChatApi.
Implementation is straightforward, no further issues.
472-472: Metadata extraction remains consistent with existing logic.
No concerns about handling this optional field.chat-android/src/main/java/com/ably/chat/OperationMetadata.kt (1)
1-11: Introduction ofOperationMetadatais well-scoped.
Using a typealias for string-based metadata clarifies usage.chat-android/src/main/java/com/ably/chat/Metadata.kt (1)
3-3: LGTM! Type alias change improves type safety.The change from
JsonElementtoJsonObjectensures metadata is always a structured object, which is more appropriate for message operations.Also applies to: 18-18
chat-android/src/test/java/com/ably/chat/integration/PresenceIntegrationTest.kt (1)
13-19: LGTM! Empty presence test is well structured.The test correctly verifies initial empty presence state.
chat-android/src/main/java/com/ably/chat/EventTypes.kt (2)
22-24: LGTM! Improved naming conventions.The renaming of
PubSubMessageNamestoPubSubEventNameandChatMessagetoCHAT_MESSAGEfollows Kotlin naming conventions for constants.
45-49: LGTM! Well-structured mapping for message actions.The internal mapping provides a type-safe way to convert between
MessageActionandMessageEventType, which is essential for the new message editing/deletion functionality.chat-android/src/main/java/com/ably/chat/Message.kt (1)
82-97: LGTM! Well-documented message versioning properties.The new properties (
version,timestamp,operation) are well-documented and properly aligned with the specs. The nullableoperationwith default null is appropriate as it's only needed for update/delete actions.chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (2)
36-37: LGTM! Comprehensive test coverage for new properties.The test cases properly validate the new
versionandtimestampproperties in the message response.Also applies to: 55-56
109-112: Consider adding test cases for operation property.While version and timestamp are tested, there are no test cases for the new
operationproperty during message updates/deletes.Would you like me to help generate additional test cases for the operation property?
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1)
79-80: LGTM! Consistent test coverage across test classes.The test assertions for
versionandtimestampare consistent with ChatApiTest.kt and use realistic values.Also applies to: 142-143
example/src/main/java/com/ably/chat/example/MainActivity.kt (1)
324-325: LGTM!The new properties
versionandtimestampare correctly added to the Message object in the preview function, with appropriate values for testing purposes.chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt (1)
144-198: Add edge case tests for message updates.The update test only covers the happy path. Consider adding tests for:
- Updating a non-existent message
- Updating a deleted message
- Concurrent updates to the same message
chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt
Show resolved
Hide resolved
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
TODO's
Update public doc/ README for usage of messages edit/delete( will be done as a part of web docs )Summary by CodeRabbit
New Features
Chores