feat: Implement Calendar Screen with event handling, tests, and updated dependencies

[othbcq]

Description

This pull request introduces the implementation of the Calendar Screen feature with dynamic event handling, multi-day and recurring event support, as well as UI tests. The PR also includes necessary dependency updates in the build files.

Key Features:

• Calendar Screen:

  • Display a calendar view with selectable dates.
  • Show events for the selected day and handle multi-day events correctly.
  • Search functionality to filter and display events by name.
  • Navigate between months with smooth transitions.

• Event Handling:

  • Support for both recurring and multi-day events.
  • Events are displayed in a list for the selected day, with details like title and date.

• UI Tests:

  • Tests for verifying the correct display of months, navigation buttons, and day selection.
  • Search button and lookup functionality tests.

• Dependency Updates:

  • Updated ical4j to version 3.0.21 for iCal event handling.
  • Added necessary Compose dependencies for runtime-livedata integration.

Changes:

• Added CalendarScreen with event display and month navigation.
• Implemented multi-day and recurring event logic in CalendarViewModel.
• Updated gradle/libs.versions.toml with required dependencies.
• Added UI tests in CalendarScreenTest.kt for verifying calendar functionality.

[sonarqubecloud]

Quality Gate failed

Failed conditions
29.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

[Kenzoud]

Great job, the code is well-structured, and the calendar feature looks great in the emulator. A few suggestions:

It would be great to add unit tests for the CalendarViewModel to cover its core functionalities, like filtering and sorting events, and also increase test coverage for the CalendarScreen.
Centering the "Look Up Event" button would improve the layout, and you could maybe add a little prompt that better describes the utility of the button (which might help users understand its purpose better).

Aside from these, everything looks solid. Great work!

[othbcq]

Great job, the code is well-structured, and the calendar feature looks great in the emulator. A few suggestions:

It would be great to add unit tests for the CalendarViewModel to cover its core functionalities, like filtering and sorting events, and also increase test coverage for the CalendarScreen. Centering the "Look Up Event" button would improve the layout, and you could maybe add a little prompt that better describes the utility of the button (which might help users understand its purpose better).

Aside from these, everything looks solid. Great work!

Hey, thanks for the feedback!

I’ll definitely add some tests for CalendarViewModel to cover its functionalities like you mentioned, and I’ll also take a look at the UI tests to improve the coverage.

For the "Look Up Event" button, I’m not sure about centering it. I feel like it might throw off the layout a bit. I was thinking of switching it to a button with a magnifying glass icon instead. It could make it more intuitive for users. What do you think?

Appreciate the suggestions! I’ll make these changes and push an update soon.

[othbcq]

I'll go ahead and merge this pull request to allow others to continue working on their parts of the project. The requested changes will be implemented as soon as possible, and I'll create a new issue to track those updates.

[Plouc314]

Overall the PR is pretty good, the description is exhaustive (although it could be a bit shorter), the code is overall pretty good, with a good structure.

However, the PR was merged even though a PR review requested changes (and was right to do so). The Request changes option acts as a big stop sign, you should always wait for an Approve review.

CalendarViewModel

• It is a good use of a view model, it is short but well designed.

CalendarScreen

• There is a good modularity of the components: CalendarScreen, SearchDialog, CalendarHeader, EventItem are all great.
• Good use of the MutableState for UI states (selectedDate, searchQuery, ...)
• Good use of callbacks (onDateSelected, onEventClick, ...)

Other

• You could consider creating a wrapper on VEvent as it pretty cumbersome to always having to get the property, perform null check, ...

This PR review is just feedback that we give you, if I chose this PR amongst others, it is because I have things to say on it, not because it is bad (it is not bad).

[Plouc314]

You should use StateFlow in your view models (you can look into the bootcamp solution for examples):

private val _icalEvents = MutableStateFlow<List<VEvent>>(listOf())
val icalEvents: StateFlow<VEvent> = query_

Sidenote when using stateflows: always use immutable structures as it only reacts to reference changes, for example:

// good
_icalEvents.value = newList

// bad
_icalEvents.value.addAll(newList)

[Plouc314]

Avoid magical values, especially when they are not trivial (1 is, MTE0OTM is not), something like this would be clearer:

fetchIcalFromUrl(ICAL_URL)

[Plouc314]

The methods handleRecurringEvents is missing documentation, both for the method itself and in the code, for example: why do we need to make a copy of eventInstance and change its DTSTART property ? In addition, the code structure is complicated with a while loop inside an if inside a for loop, consider refactoring or adding comments.
Also, the function takes a mutable list allEvents as argument and modify it, while this is good for performance (i.e. it's what you would do in C), it is something you want to avoid, as the function is then performing side-effects, which are known to introduce bugs and be harder to reason about.

Here is an example of how handleRecurringEvents could be refactored:

  /**
   * General doc about the function
   */
  private fun handleRecurringEvents(
      component: VEvent,
      period: Period,
      eventStartDate: Date,
      eventEndDate: Date,
  ): List<VEvent> {
    val events = mutableListOf<VEvent>()
    val recurrenceSet = component.calculateRecurrenceSet(period)

    for (recurrence in recurrenceSet) {
      val eventInstance = component.copy() as VEvent
      eventInstance.getProperty<DtStart>(Property.DTSTART)?.date = recurrence.start

      if (!eventEndDate.after(recurrence.start)) {
        // comment on why no while loop
        events.add(eventInstance)
        continue
      }

      var current = recurrence.start
      while (current.before(eventEndDate)) {

        // comment on why this
        val multiDayEventInstance = eventInstance.copy() as VEvent
        multiDayEventInstance.getProperty<DtStart>(Property.DTSTART)?.date =
            DateTime(current.time)
        events.add(multiDayEventInstance)

        // comment add a day, or better use an helper function
        val calendarInstance =
            JavaCalendar.getInstance().apply {
              time = current
              add(JavaCalendar.DATE, 1)
            }
        current = DateTime(calendarInstance.time)
      }
    }
    return events
  }

[Plouc314]

Same comment as for handleRecurringEvents method, it is missing documentation and taking a mutable list as argument.

[Plouc314]

This try-catch block is very long and it catches all exceptions subtype of Exception, try to isolate the code which may throw the exception and restrict the catch block to catch more specific exception if applicable (for example IOException)

[Plouc314]

Instead of having both EventListWithResults and EventList components, you could have one component that would take a title, list of events, and onEventClick as arguments, to have a more modular approach, something like that:

@Composable
fun EventList(title: String, events: List<VEvent>, onEventClick: (Date) -> Unit) {
  Column(modifier = Modifier.fillMaxWidth().padding(16.dp)) {
    Text(
        text = title,
        style = MaterialTheme.typography.titleMedium,
        fontWeight = FontWeight.Bold,
        modifier = Modifier.padding(bottom = 8.dp))

    if (events.isNotEmpty()) {
      events.forEach { event ->
        val eventDate = event.getProperty<DtStart>(Property.DTSTART)?.date as Date
        EventItem(event = event, onClick = { onEventClick(eventDate) })
        Spacer(modifier = Modifier.height(8.dp))
      }
    } else {
      Text(
          text = "No events.",
          style = MaterialTheme.typography.bodyMedium,
          color = MaterialTheme.colorScheme.onSurfaceVariant)
    }
  }
}

[Plouc314]

Instead of passing the view model instance to then use the icalEvents attribute, consider passing a list of VEvent, icalEvents. The idea is to pass the least general object down, as it then clear that CalendarGrid only need the ical events list to someone not familiar with CalendarGrid and it is simplier to test.

[Plouc314]

Consider creating a component here, to simplify the CalendarGrid method.

[Plouc314]

Consider creating a DateUtils module to group all helper functions for dates, so that they can be reused in other files. That way you can also keep track of all already defined helper functions to avoid duplicates.

[Plouc314]

There's nothing more permanent than a temporary solution - Oswald Maskens (I must give credits to the guy)

Either create the tests now or don't basically.