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.

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

[incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour #3795

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

martinweiler
Copy link
Contributor

Copy link
Contributor

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

@martinweiler code looks good to me, especially big kudos for the tests. I have one comment inline.

@@ -293,7 +299,9 @@ public Date calculateBusinessTimeAsDate(String timeExpression) {
c.set(Calendar.HOUR_OF_DAY, startHour);
c.add(Calendar.HOUR_OF_DAY, currentCalHour - endHour);
} else if (currentCalHour < startHour) {
c.add(Calendar.HOUR_OF_DAY, startHour);
c.add(Calendar.HOUR_OF_DAY, startHour - currentCalHour);
Copy link
Contributor

Choose a reason for hiding this comment

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

As we change construction of HOUR_OF_DAY, maybe it would be worth to add also *hour* tests?

During my brief check, I didn't find such tests. Also, I didn't check if tring duration = "2h"; is a correct duration value. I wrote the code below just in github comment.

If you think, such tests are not needed, feel free to proceed without adding such tests.

    @Test
    public void testCalculateHoursBeforeStartHour() {
        Properties config = new Properties();
        config.setProperty(BusinessCalendarImpl.HOURS_PER_DAY, "4");
        config.setProperty(BusinessCalendarImpl.START_HOUR, "14");
        config.setProperty(BusinessCalendarImpl.END_HOUR, "18");
        String currentDate = "2024-11-28 10:48:33.000";
        String duration = "2h";
        String expectedDate = "2024-11-28 16:00:00";

        SessionPseudoClock clock = new StaticPseudoClock(parseToDateWithTimeAndMillis(currentDate).getTime());
        BusinessCalendarImpl businessCal = new BusinessCalendarImpl(config, clock);

        Date result = businessCal.calculateBusinessTimeAsDate(duration);

        assertThat(formatDate("yyyy-MM-dd HH:mm:ss", result)).isEqualTo(expectedDate);
    }


    @Test
    public void testCalculateHoursBeforeEndHour() {
        Properties config = new Properties();
        config.setProperty(BusinessCalendarImpl.HOURS_PER_DAY, "4");
        config.setProperty(BusinessCalendarImpl.START_HOUR, "14");
        config.setProperty(BusinessCalendarImpl.END_HOUR, "18");
        String currentDate = "2024-11-28 17:58:33.000";
        String duration = "2h";
        String expectedDate = "2024-11-29 15:58:33";

        SessionPseudoClock clock = new StaticPseudoClock(parseToDateWithTimeAndMillis(currentDate).getTime());
        BusinessCalendarImpl businessCal = new BusinessCalendarImpl(config, clock);

        Date result = businessCal.calculateBusinessTimeAsDate(duration);

        assertThat(formatDate("yyyy-MM-dd HH:mm:ss", result)).isEqualTo(expectedDate);
    }

@yesamer
Copy link
Contributor

yesamer commented Nov 29, 2024

FYI @Abhitocode @gitgabrio this could create a conflict with the refactoring you're doing

@gitgabrio
Copy link
Contributor

gitgabrio commented Nov 29, 2024

@martinweiler
As you know, @Abhitocode and me are actively refactoring that whole class, so we may include that modification also in the other PR.
@jomarko
about testing, see linked PR

@jomarko
Copy link
Contributor

jomarko commented Nov 29, 2024

@gitgabrio hi, not sure if your point is:

  1. the tests I suggest (their version) is already present in the other PR BusinessCalendarImplTest.java
  2. or I should provide same tests I did here in the style of the other PR
  3. or it was just FYI that it will be done there without my input
  4. or ...

Sorry, just want to be sure what I should do. Thank you for clarification.

@gitgabrio
Copy link
Contributor

@jomarko
I jmeant that, if you have time, you could take a look at the tests we are doing in the other PR, to see if those already cover your suggestion or you think something is missing also there

@tiagobento tiagobento merged commit 2210a32 into apache:main Dec 3, 2024
6 checks passed
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.

Incorrect calculation in BusinessCalendarImpl when currentHour < configured startHour
6 participants