Skip to content

Conversation

@tamaskozmer
Copy link
Contributor

No description provided.

tamaskozmer and others added 3 commits December 17, 2025 14:12
* Fixed duplicate key issue

* Distinct by id

* Add distinctBy to other places.
Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Review Summary

This PR addresses duplicate planner items appearing in the todo list by adding distinctBy { it.id } filtering across multiple locations. The implementation is solid with excellent test coverage.

Positive Aspects

Consistent Implementation - The deduplication logic is applied consistently across all relevant locations (CallbackActivity, ToDoWidgetUpdater, and ToDoListViewModel)

Excellent Test Coverage - Five comprehensive test cases cover various duplicate scenarios:

  • Single duplicate items
  • Multiple duplicates of the same item
  • Duplicates across different date groups
  • Verification that first occurrence is preserved
  • Multiple different duplicates

Clean Solution - Using distinctBy { it.id } is idiomatic Kotlin and performs well

Version Bump - Properly incremented version from 8.4.1 to 8.4.2

API Updates - Updated service URLs to new infrastructure (core.inseng.io)

Items to Consider

  • Filter Order - Verify that applying distinctBy after filtering announcements/assessment requests is intentional. If duplicates can exist with different plannable types (e.g., ID=1 as ANNOUNCEMENT and ID=1 as ASSIGNMENT), the current order keeps non-filtered types. See inline comments on ToDoListViewModel.kt:152 and CallbackActivity.kt:269

Code Quality Assessment

  • No bugs detected - Logic is sound
  • No security concerns - Changes are data filtering only
  • Performance - distinctBy is O(n) and appropriate for this use case
  • No missing edge cases - Test coverage is thorough

The duplicate filtering strategy (keeping first occurrence) is reasonable and well-tested. Great work on the comprehensive test suite!

Copy link
Contributor

@kdeakinstructure kdeakinstructure left a comment

Choose a reason for hiding this comment

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

QA 👍

@tamaskozmer tamaskozmer merged commit 232961a into release/student Dec 18, 2025
4 checks passed
@tamaskozmer tamaskozmer deleted the release/student-8.4.2-285 branch December 18, 2025 15:23
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