You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "martinweiler (via GitHub)" <gi...@apache.org> on 2024/11/28 18:06:13 UTC
[PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
martinweiler opened a new pull request, #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795
Closes: https://github.com/apache/incubator-kie-issues/issues/1648
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
Posted by "jomarko (via GitHub)" <gi...@apache.org>.
jomarko commented on code in PR #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795#discussion_r1862985264
##########
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java:
##########
@@ -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);
Review Comment:
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);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on PR #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795#issuecomment-2507201491
FYI @Abhitocode @gitgabrio this could create a conflict with the refactoring you're doing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
Posted by "jomarko (via GitHub)" <gi...@apache.org>.
jomarko commented on PR #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795#issuecomment-2507237419
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795#discussion_r1863313932
##########
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java:
##########
@@ -251,7 +253,9 @@ public Date calculateBusinessTimeAsDate(String timeExpression) {
c.set(Calendar.MINUTE, 0);
c.set(Calendar.SECOND, 0);
} else if (currentCalHour < startHour) {
- c.add(Calendar.HOUR_OF_DAY, startHour);
+ c.add(Calendar.HOUR_OF_DAY, startHour - currentCalHour);
+ c.set(Calendar.MINUTE, 0);
Review Comment:
HI @martinweiler
I have the impression that setting MINUTE and SECOND to = 0 could be a problem, because
1) c.add(Calendar.HOUR_OF_DAY, startHour); <. this is meant to push the calendar forward, to match the current hour
2) bat setting MINUTE and SECONDS to = 0 pushe the calendar back; so after them the current calendar is "before" the start hour
I'm testing that modification in the other PR, and this is what I found there - I'll keep investigating, anyway, to double check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on PR #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795#issuecomment-2507254246
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
Posted by "tiagobento (via GitHub)" <gi...@apache.org>.
tiagobento merged PR #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795#discussion_r1863313932
##########
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java:
##########
@@ -251,7 +253,9 @@ public Date calculateBusinessTimeAsDate(String timeExpression) {
c.set(Calendar.MINUTE, 0);
c.set(Calendar.SECOND, 0);
} else if (currentCalHour < startHour) {
- c.add(Calendar.HOUR_OF_DAY, startHour);
+ c.add(Calendar.HOUR_OF_DAY, startHour - currentCalHour);
+ c.set(Calendar.MINUTE, 0);
Review Comment:
HI @martinweiler
I have the impression that setting MINUTE and SECOND to = could be a problem, because
1) c.add(Calendar.HOUR_OF_DAY, startHour); <. this is meant to push the calendar forward, to match the current hour
2) bat setting MINUTE and SECONDS to = 0 pushe the calendar back; so after them the current calendar is "before" the start hour
I'm testing that modification in the other PR, and this is what I found there - I'll keep investigating, anyway, to double check
##########
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java:
##########
@@ -267,7 +271,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);
+ c.set(Calendar.MINUTE, 0);
Review Comment:
@martinweiler
See previous comment
##########
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java:
##########
@@ -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);
Review Comment:
@martinweiler
See my previous comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795#discussion_r1863313932
##########
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java:
##########
@@ -251,7 +253,9 @@ public Date calculateBusinessTimeAsDate(String timeExpression) {
c.set(Calendar.MINUTE, 0);
c.set(Calendar.SECOND, 0);
} else if (currentCalHour < startHour) {
- c.add(Calendar.HOUR_OF_DAY, startHour);
+ c.add(Calendar.HOUR_OF_DAY, startHour - currentCalHour);
+ c.set(Calendar.MINUTE, 0);
Review Comment:
HI @martinweiler
I have the impression that setting MINUTE and SECOND to = 0 could be a problem, because
1) c.add(Calendar.HOUR_OF_DAY, startHour); <. this is meant to push the calendar forward, to match the current hour
2) but setting MINUTE and SECONDS to = 0 pushes the calendar back; so after them the current calendar is "before" the start hour
I'm testing that modification in the other PR, and this is what I found there - I'll keep investigating, anyway, to double check
##########
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java:
##########
@@ -267,7 +271,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);
+ c.set(Calendar.MINUTE, 0);
Review Comment:
@martinweiler
See previous comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on PR #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795#issuecomment-2507222025
@martinweiler
As you know, @Abhitocode and me are actively refactoring that whole class, so we may include that modification in the [other PR](https://github.com/apache/incubator-kie-kogito-runtimes/pull/3788).
@jomarko
about testing, see linked PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795#discussion_r1863313932
##########
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java:
##########
@@ -251,7 +253,9 @@ public Date calculateBusinessTimeAsDate(String timeExpression) {
c.set(Calendar.MINUTE, 0);
c.set(Calendar.SECOND, 0);
} else if (currentCalHour < startHour) {
- c.add(Calendar.HOUR_OF_DAY, startHour);
+ c.add(Calendar.HOUR_OF_DAY, startHour - currentCalHour);
+ c.set(Calendar.MINUTE, 0);
Review Comment:
HI @martinweiler
I have the impression that setting MINUTE and SECOND to = 0 could be a problem, because
1) c.add(Calendar.HOUR_OF_DAY, startHour); <. this is meant to push the calendar forward, to match the current hour
2) but setting MINUTE and SECONDS to = 0 pushes the calendar back; so after them the current calendar is "before" the start hour
I'm testing that modification in the other PR, and this is what I found there - I'll keep investigating, anyway, to double check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#1648] Fix BusinessCalendarImpl time calculation when currentCalHour < startHour [incubator-kie-kogito-runtimes]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #3795:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3795#discussion_r1863314657
##########
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java:
##########
@@ -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);
Review Comment:
@martinweiler
See my previous comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org