Merge pull request #375 from hossain-khan/372-fix-working-hour

[FIXED] Working time issue when it is started on weekend

src/main/kotlin/CodeSnippets.kt

@@ -44,5 +44,5 @@ fun main() {
 //    }
 
     // Show disclaimer about the review time being inaccurate
-    Log.i(Art.warnAboutReviewTime())
+    Log.w(Art.warnAboutReviewTime())
 }

src/main/kotlin/Main.kt

@@ -32,5 +32,5 @@ fun main() {
     }
 
     // Show disclaimer about the review time being inaccurate
-    Log.i(Art.warnAboutReviewTime())
+    Log.w(Art.warnAboutReviewTime())
 }

src/main/kotlin/dev/hossain/githubstats/repository/PullRequestStatsRepoImpl.kt

@@ -299,7 +299,7 @@ class PullRequestStatsRepoImpl(
     }
 
     /**
-     * Evaluates actual time when PR was available for use to review by considering
+     * Evaluates actual time when PR was available for user to review by considering
      * if review was requested later from the specified [reviewer].
      * Or, if user self reviewed PR without being added, then the original time will be used.
      */

src/main/kotlin/dev/hossain/time/DateTimeDiffer.kt

@@ -72,23 +72,44 @@ object DateTimeDiffer {
             // Loop through all dates and sums up only the working hours on working day.
             else -> {
                 var workingHours = Duration.ZERO
-                var previousWorkingDay =
-                    if (startDateTime.isAfterWorkingHour()) startDateTime.nextWorkingHourOrSame() else startDateTime
+                var previousWorkingDay = if (startDateTime.isAfterWorkingHour()) startDateTime.nextWorkingHourOrSame() else startDateTime
                 var immediateNextWorkingDay = previousWorkingDay.nextNonWorkingHour()
 
+                // Debug date time used in the calculation - Keep it commented out in production
+
                 /*println("startDateTime           = ${startDateTime.format()},\n" +
                         "endDateTime             = ${endDateTime.format()},\n" +
                         "previousWorkingDay      = ${previousWorkingDay.format()},\n" +
                         "immediateNextWorkingDay = ${immediateNextWorkingDay.format()},\n" +
-                        "immediateNextWorkingDay.isBefore(endDateTime)=${immediateNextWorkingDay.isBefore(endDateTime)},\n" +
-                        "!immediateNextWorkingDay.isSameDay(endDateTime)=${!immediateNextWorkingDay.isSameDay(endDateTime)}")*/
+                        "immediateNextWorkingDay.isBefore(endDateTime)         = ${immediateNextWorkingDay.isBefore(endDateTime)},\n" +
+                        "!immediateNextWorkingDay.isSameDay(endDateTime)       = ${!immediateNextWorkingDay.isSameDay(endDateTime)},\n" +
+                        "previousWorkingDay.isSameDay(immediateNextWorkingDay) = ${previousWorkingDay.isSameDay(immediateNextWorkingDay)},\n" +
+                        "previousWorkingDay.isOnWorkingDay().not()             = ${previousWorkingDay.isOnWorkingDay().not()},\n" +
+                        "\n")*/
 
                 // Loop through the dates while `immediateNextWorkingDay` is before end date and is not same day
                 while (immediateNextWorkingDay.isBefore(endDateTime) && !immediateNextWorkingDay.isSameDay(endDateTime)) {
                     if (previousWorkingDay.isSameDay(immediateNextWorkingDay) &&
                         previousWorkingDay.isOnWorkingDay().not()
                     ) {
-                        // Skip calculating weekends
+                        // Debug date time used in the calculation - Keep it commented out in production
+
+                        /*println("Skipping weekend calculations. Overriding following:\n" +
+                                "previousWorkingDay      = ${previousWorkingDay.format()} -> to -> ${previousWorkingDay.nextWorkingDay().format()} +/- start of the day\n" +
+                                "immediateNextWorkingDay = ${immediateNextWorkingDay.format()} -> to -> ${immediateNextWorkingDay.nextWorkingDay().format()}\n" +
+                                "\n")*/
+
+                        // Handles the case where next working day after weekend may not have a start time
+                        // that is within working hours. So, we need to adjust it to the next working hour appropriately.
+                        val nextWorkingDayAfterWeekend = previousWorkingDay.nextWorkingDay()
+                        previousWorkingDay =
+                            if (nextWorkingDayAfterWeekend.isBeforeWorkingHour()) {
+                                nextWorkingDayAfterWeekend.nextWorkingHourOrSame()
+                            } else {
+                                nextWorkingDayAfterWeekend.prevWorkingHour()
+                            }
+
+                        // Skip calculating weekends - just move to next working day that is required to calculate working hours
                         previousWorkingDay = previousWorkingDay.nextWorkingDay().prevWorkingHour()
                         immediateNextWorkingDay = immediateNextWorkingDay.nextWorkingDay()
                         continue
@@ -111,15 +132,22 @@ object DateTimeDiffer {
     }
 
     /**
-     * Provides working day work hour duration between
-     * two working dates denoted by [startDateTime] and [endDateTime].
+     * Provides working day work hour duration between two working dates denoted by [startDateTime] and [endDateTime].
+     *
+     * NOTE: This function assumes that both [startDateTime] and [endDateTime] are on working days.
+     * If either of the date-time is not on a working day, it throws an [IllegalArgumentException].
      */
     private fun workingDuration(
         startDateTime: ZonedDateTime,
         endDateTime: ZonedDateTime,
     ): Duration {
+        // Checks if both start and end date-times are on working days.
+        // Throws an IllegalArgumentException if either of the date-times is not on a working day.
         if ((startDateTime.isOnWorkingDay() && endDateTime.isOnWorkingDay()).not()) {
-            throw IllegalArgumentException("This function can only handle working day diff")
+            throw IllegalArgumentException(
+                "This function can only handle working day diff. " +
+                    "Start: $startDateTime, End: $endDateTime - both must be on working day.",
+            )
         }
 
         val startToEndDiff = startDateTime.diffWith(endDateTime)

src/main/kotlin/dev/hossain/time/TemporalsExtension.kt

@@ -117,7 +117,17 @@ object TemporalsExtension {
 
         /**
          * Adjuster that gives next end of day working hour or same if it's already non-working hour.
+         *
          * TODO - consider day too
+         *
+         * Example:
+         *  - Current Date Time: Monday 3 PM  --> Monday 5 PM (shift to after 5:00 PM)
+         *  - Current Date Time: Tuesday 10 AM --> Tuesday 5 PM (shift to after 5:00 PM)
+         *  - Current Date Time: Wednesday 6 PM --> Wednesday 6 PM (same, already non-working hour)
+         *  - Current Date Time: Thursday 8 PM --> Thursday 8 PM (same, already non-working hour)
+         *  - Current Date Time: Friday 4 PM --> Friday 5 PM (shift to after 5:00 PM)
+         *  - Current Date Time: Saturday 11 AM --> Saturday 11 AM (same, already non-working hour)
+         *  - Current Date Time: Sunday 2 PM --> Sunday 2 PM (same, already non-working hour)
          */
         NEXT_NON_WORKING_HOUR_OR_SAME {
             override fun adjustInto(temporal: Temporal): Temporal =

src/main/kotlin/dev/hossain/time/ZonedDateTimeExtension.kt

@@ -49,6 +49,18 @@ internal fun ZonedDateTime.nextWorkingDayOrSame() = this.with(TemporalsExtension
  */
 internal fun ZonedDateTime.nextWorkingHourOrSame() = this.with(TemporalsExtension.nextWorkingHourOrSame())
 
+/**
+ * Provides the next non-working hour for the current date-time.
+ * If the current date-time is already within non-working hours, it returns the same date-time.
+ *
+ * Example:
+ *  - Current Date Time: Monday 11 AM   --> Monday 5 PM (End of the day)
+ *  - Current Date Time: Saturday 11 AM --> Saturday 11 AM (Same - because on non-weekday)
+ *  - Current Date Time: Sunday 11 AM   --> Sunday 11 AM (Same - because on non-weekday)
+ *  - Current Date Time: Monday 6 PM    --> Monday 6 PM (Same - because it's after working hours)
+ *  - Current Date Time: Tuesday 8 PM   --> Tuesday 8 PM (Same - because it's after working hours)
+ *  - Current Date Time: Tuesday 6 AM   --> Tuesday 6 AM (Same - because it's before working hours)
+ */
 internal fun ZonedDateTime.nextNonWorkingHour() = this.with(TemporalsExtension.nextNonWorkingHourOrSame())
 
 /**

src/test/kotlin/dev/hossain/githubstats/PullRequestStatsRepoTest.kt

@@ -260,6 +260,8 @@ internal class PullRequestStatsRepoTest {
         runTest {
             // Uses data from https://github.com/freeCodeCamp/freeCodeCamp/pull/47550
             // A lot of review comments by 5 people, and 2 people approved after dismissal
+            // PR was opened on weekend @ Saturday, Sep 17, 2022 at 6pm
+            // Most approval was        @ Thursday, Sept 22, 2022 at 5:30pm
             mockWebServer.enqueue(MockResponse().setBody(respond("pulls-freeCodeCamp-47550.json")))
             mockWebServer.enqueue(MockResponse().setBody(respond("timeline-freeCodeCamp-47550.json")))
             mockWebServer.enqueue(MockResponse().setBody("[]")) // PR Review comments
@@ -277,6 +279,36 @@ internal class PullRequestStatsRepoTest {
             assertThat(initialResponseTime["jeremylt"]).isEqualTo(Duration.parse("2h 49m"))
             assertThat(initialResponseTime["naomi-lgbt"]).isEqualTo(Duration.parse("8h"))
             assertThat(initialResponseTime["Sboonny"]).isEqualTo(Duration.parse("1d"))
+
+            val prApprovalTime = result.stats.prApprovalTime
+            assertThat(prApprovalTime).hasSize(2)
+            assertThat(prApprovalTime["naomi-lgbt"]).isEqualTo(Duration.parse("1d 8h"))
+            assertThat(prApprovalTime["ShaunSHamilton"]).isEqualTo(Duration.parse("1d 8h"))
+        }
+
+    @Test
+    fun `stats - given PR opened on weekend and approved by 2 reviewers - provides correct review time`() =
+        runTest {
+            // PR opened on weekend and approved by 2 reviewers
+            // Uses data from https://github.com/freeCodeCamp/freeCodeCamp/pull/56555
+            mockWebServer.enqueue(MockResponse().setBody(respond("pulls-freeCodeCamp-56555.json")))
+            mockWebServer.enqueue(MockResponse().setBody(respond("timeline-freeCodeCamp-56555.json")))
+            mockWebServer.enqueue(MockResponse().setBody("[]")) // PR Review comments
+
+            val statsResult = pullRequestStatsRepo.stats(REPO_OWNER, REPO_ID, 123, emptyList())
+
+            assertThat(statsResult).isInstanceOf(StatsResult.Success::class.java)
+
+            val result = statsResult as StatsResult.Success
+            val initialResponseTime = result.stats.initialResponseTime
+            val prApprovalTime = result.stats.prApprovalTime
+            assertThat(initialResponseTime).hasSize(2)
+
+            assertThat(initialResponseTime["naomi-lgbt"]).isEqualTo(Duration.parse("8h"))
+            assertThat(initialResponseTime["gikf"]).isEqualTo(Duration.parse("20h 11m"))
+
+            assertThat(prApprovalTime["gikf"]).isEqualTo(Duration.parse("20h 11m"))
+            assertThat(prApprovalTime["naomi-lgbt"]).isEqualTo(Duration.parse("19h 53m"))
         }
 
     @Test

src/test/kotlin/dev/hossain/time/DateTimeDifferTest.kt

@@ -379,5 +379,19 @@ internal class DateTimeDifferTest {
         assertThat(diffWorkingHours).isEqualTo("15h".duration())
     }
 
+    @Test
+    fun `diffWorkingHours - given start time is in weekend and end time is few days later - provides diff of working hour only`() {
+        // This function can only handle working day diff - both must be on working day.
+        // Start: 2024-10-06T17:00:03-04:00[America/New_York]
+        // End: 2024-10-07T17:15:03-04:00[America/New_York]
+
+        val startTime: Instant = Instant.parse("2024-10-06T17:00:03-04:00") // Sunday, Oct 6, 2024, 5:00:03 p.m.
+        val endTime: Instant = Instant.parse("2024-10-07T17:15:03-04:00") // Monday, Oct 7, 2024, 5:15:03 p.m.
+
+        val diffWorkingHours = DateTimeDiffer.diffWorkingHours(startTime, endTime, zoneId)
+
+        assertThat(diffWorkingHours).isEqualTo("8h".duration())
+    }
+
     private fun String.duration(): Duration = Duration.parse(this)
 }