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.

Sign up for GitHub

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

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

Merged
merged 5 commits into from
Oct 13, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d2a745b
feat(calendar): Implement dynamic calendar with event handling
Oct 12, 2024
d35d87c
build: Update build.gradle with necessary dependencies and configurat…
Oct 12, 2024
e9bb9d9
chore: Add calendar and compose dependencies to libs.versions.toml
Oct 12, 2024
ebc2112
test(calendar): Add UI tests for CalendarScreen functionality
Oct 12, 2024
1962587
feat(model): Add model classes for calendar functionality
Oct 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion app/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,9 @@ dependencies {
implementation(libs.androidx.appcompat)
implementation(libs.material)
implementation(libs.androidx.lifecycle.runtime.ktx)
implementation(libs.androidx.navigation.runtime.ktx)

// Jetpack Compose BOM
// Jetpack Compose BOM
val composeBom = platform(libs.compose.bom)
implementation(composeBom)
androidTestImplementation(composeBom)
Expand Down Expand Up @@ -190,6 +191,12 @@ dependencies {
// Networking with OkHttp
implementation(libs.okhttp)

// Calendar
implementation(libs.ical4j)
implementation(libs.compose)
implementation("com.prolificinteractive:material-calendarview:1.4.3") {
exclude(group = "com.android.support", module = "support-v4")
}
}

tasks.withType<Test> {
Expand Down
92 changes: 92 additions & 0 deletions app/src/androidTest/java/com/github/lookupgroup27/lookup/screen/CalendarScreenTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package com.github.lookupgroup27.lookup.screen

import androidx.compose.ui.test.assertIsDisplayed
import androidx.compose.ui.test.junit4.createComposeRule
import androidx.compose.ui.test.onNodeWithText
import androidx.compose.ui.test.performClick
import com.github.lookupgroup27.lookup.model.calendar.CalendarViewModel
import com.github.lookupgroup27.lookup.ui.navigation.NavigationActions
import com.github.lookupgroup27.lookup.ui.overview.CalendarScreen
import io.mockk.mockk
import java.text.SimpleDateFormat
import java.util.*
import org.junit.Before
import org.junit.Rule
import org.junit.Test

class CalendarScreenTest {

private lateinit var calendarViewModel: CalendarViewModel
private lateinit var navigationActions: NavigationActions
private val dateFormat = SimpleDateFormat("MMMM yyyy", Locale.getDefault())

@get:Rule val composeTestRule = createComposeRule()

@Before
fun setUp() {

calendarViewModel = mockk(relaxed = true)
navigationActions = mockk(relaxed = true)

composeTestRule.setContent {
CalendarScreen(calendarViewModel = calendarViewModel, navigationActions = navigationActions)
}
}

@Test
fun testCalendarHeaderDisplaysCurrentMonth() {
val currentDate = Calendar.getInstance().time
val formattedMonth = dateFormat.format(currentDate)

composeTestRule.onNodeWithText(formattedMonth).assertIsDisplayed()
}

@Test
fun testPreviousMonthButton() {
val currentDate = Calendar.getInstance().time
val previousMonthDate =
Calendar.getInstance()
.apply {
time = currentDate
add(Calendar.MONTH, -1)
}
.time
val formattedPreviousMonth = dateFormat.format(previousMonthDate)

composeTestRule.onNodeWithText("<").performClick()
composeTestRule.onNodeWithText(formattedPreviousMonth).assertIsDisplayed()
}

@Test
fun testNextMonthButton() {
val currentDate = Calendar.getInstance().time
val nextMonthDate =
Calendar.getInstance()
.apply {
time = currentDate
add(Calendar.MONTH, 1)
}
.time
val formattedNextMonth = dateFormat.format(nextMonthDate)

composeTestRule.onNodeWithText(">").performClick()
composeTestRule.onNodeWithText(formattedNextMonth).assertIsDisplayed()
}

@Test
fun testLookUpEventButtonExists() {
composeTestRule.onNodeWithText("Look Up Event").assertIsDisplayed()
}

// Add more tests when figma is updated with all the components and testTags
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


@Test
fun testSelectingDayUpdatesSelectedDate() {
val calendar = Calendar.getInstance()
val dayToSelect = calendar.get(Calendar.DAY_OF_MONTH) + 1

composeTestRule.onNodeWithText(dayToSelect.toString()).performClick()

composeTestRule.onNodeWithText(dayToSelect.toString()).assertIsDisplayed()
}
}
148 changes: 148 additions & 0 deletions app/src/main/java/com/github/lookupgroup27/lookup/model/calendar/CalendarViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
package com.github.lookupgroup27.lookup.model.calendar

import android.util.Log
import androidx.compose.runtime.mutableStateListOf
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import java.io.StringReader
import java.util.Calendar as JavaCalendar
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import net.fortuna.ical4j.data.CalendarBuilder
import net.fortuna.ical4j.model.Calendar
import net.fortuna.ical4j.model.Date
import net.fortuna.ical4j.model.DateTime
import net.fortuna.ical4j.model.Period
import net.fortuna.ical4j.model.Property
import net.fortuna.ical4j.model.component.VEvent
import net.fortuna.ical4j.model.property.DtEnd
import net.fortuna.ical4j.model.property.DtStart
import net.fortuna.ical4j.model.property.RRule
import net.fortuna.ical4j.util.MapTimeZoneCache
import okhttp3.OkHttpClient
import okhttp3.Request

class CalendarViewModel : ViewModel() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may want to create a IcalRepository interface to pass as an argument to the view model, create HttpIcalRepository and MockIcalRepository classes to be able to easily test this view model.
Alternatively you could also take an OkHttpClient instance as argument and mock it when testing, but that could be a little more complicated to do.


private val client = OkHttpClient()
private val _icalEvents = mutableStateListOf<VEvent>()
val icalEvents: List<VEvent> = _icalEvents
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)


init {
System.setProperty("net.fortuna.ical4j.timezone.cache.impl", MapTimeZoneCache::class.java.name)
fetchICalData()
}

fun fetchICalData() {
viewModelScope.launch {
val icalData =
fetchIcalFromUrl(
"https://p127-caldav.icloud.com/published/2/MTE0OTM4OTk2MTExNDkzOIzDRaBjEGa9_1mlmgjzcdlka5HK6EzMiIdOswU-0rZBZMDBibtH_M7CDyMpDQRQJPdGOSM0hTsS2qFNGOObsTc")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

fetchIcalFromUrl(ICAL_URL)

icalData?.let { parseICalData(it) }
}
}

private suspend fun fetchIcalFromUrl(url: String): String? =
withContext(Dispatchers.IO) {
return@withContext try {
val request = Request.Builder().url(url).build()
val response = client.newCall(request).execute()
response.body?.string()
} catch (e: Exception) {
Log.e("CalendarViewModel", "Error fetching iCal data: ${e.localizedMessage}", e)
null
}
}

private fun parseICalData(icalData: String) {
try {
val reader = StringReader(icalData)
val calendar: Calendar = CalendarBuilder().build(reader)
val start = DateTime(System.currentTimeMillis())
val end = DateTime(System.currentTimeMillis() + 365L * 24 * 60 * 60 * 1000)
val period = Period(start, end)

val allEvents = mutableListOf<VEvent>()

for (component in calendar.getComponents<VEvent>(VEvent.VEVENT)) {
val dtStart = component.getProperty<DtStart>(Property.DTSTART)
val dtEnd = component.getProperty<DtEnd>(Property.DTEND)

dtStart?.let { startDate ->
val endDate = dtEnd?.date ?: startDate.date
val rrule = component.getProperty<RRule>(Property.RRULE)

if (rrule != null) {
handleRecurringEvents(component, period, startDate.date, endDate, allEvents)
} else if (startDate.date.before(end) && endDate.after(start)) {
handleNonRecurringEvents(component, startDate.date, endDate, allEvents)
}
}
}

_icalEvents.clear()
_icalEvents.addAll(allEvents)
Log.d("CalendarViewModel", "Total events parsed: ${_icalEvents.size}")
} catch (e: Exception) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Log.e("CalendarViewModel", "Error parsing iCal data: ${e.localizedMessage}", e)
}
}

private fun handleRecurringEvents(
component: VEvent,
period: Period,
eventStartDate: Date,
eventEndDate: Date,
allEvents: MutableList<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)) {
var current = recurrence.start
while (current.before(eventEndDate)) {
val multiDayEventInstance = eventInstance.copy() as VEvent
multiDayEventInstance.getProperty<DtStart>(Property.DTSTART)?.date =
DateTime(current.time)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
  }

allEvents.add(multiDayEventInstance)

val calendarInstance =
JavaCalendar.getInstance().apply {
time = current
add(JavaCalendar.DATE, 1)
}
current = DateTime(calendarInstance.time)
}
} else {
allEvents.add(eventInstance)
}
}
}

private fun handleNonRecurringEvents(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

component: VEvent,
eventStartDate: Date,
eventEndDate: Date,
allEvents: MutableList<VEvent>
) {
if (eventEndDate.after(eventStartDate)) {
var current = eventStartDate
while (current.before(eventEndDate)) {
val eventInstance = component.copy() as VEvent
eventInstance.getProperty<DtStart>(Property.DTSTART)?.date = DateTime(current.time)
allEvents.add(eventInstance)

val calendarInstance =
JavaCalendar.getInstance().apply {
time = current
add(JavaCalendar.DATE, 1)
}
current = DateTime(calendarInstance.time)
}
} else {
allEvents.add(component)
}
}
}
Loading