Skip to content

feat: unenrolment should be available only over 48h before occurrence#693

Merged
nikomakela merged 1 commit into
mainfrom
KK-1436-48h-cancel-limit
Jun 3, 2025
Merged

feat: unenrolment should be available only over 48h before occurrence#693
nikomakela merged 1 commit into
mainfrom
KK-1436-48h-cancel-limit

Conversation

@nikomakela

Copy link
Copy Markdown
Contributor

KK-1436.

Add AppConfig.enrolmentCancellationTimeLimitHours that can be used to configure the time (in hours) that limits the cancellation time.

nikomakela added a commit to City-of-Helsinki/kukkuu that referenced this pull request May 29, 2025
KK-1436.

Configure `KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE` with environment
variable and use it to limit unenrolments too late before an occurrence
start time. The default value is set to 48h. Use 0 as a value to disable
this new feature.

The UI changes are implemented in
City-of-Helsinki/kukkuu-ui#693.
@nikomakela

nikomakela commented May 29, 2025

Copy link
Copy Markdown
Contributor Author

Some texts are still needed to informate about the 48h limit

  • A text should say why the unenrolment button is disabled.
  • A text in event should clearly say when the unenrolments are needed to be done.
  • Different text for internal and external enrolments, (or no text for external)
  • Approve texts from product owner

@nikomakela nikomakela self-assigned this May 29, 2025
@nikomakela nikomakela added the Comodori Team Comodori's projected work. label May 29, 2025
@terovirtanen

Copy link
Copy Markdown
Contributor

KUKKUU-UI branch is deployed to platta: https://kukkuu-ui-pr693.dev.hel.ninja 🚀🚀🚀

@terovirtanen

Copy link
Copy Markdown
Contributor

TestCafe result is success for https://kukkuu-ui-pr693.dev.hel.ninja 😆🎉🎉🎉

nikomakela added a commit to City-of-Helsinki/kukkuu that referenced this pull request May 30, 2025
KK-1436.

Configure `KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE` with environment
variable and use it to limit unenrolments too late before an occurrence
start time. The default value is set to 48h. Use 0 as a value to disable
this new feature.

The UI changes are implemented in
City-of-Helsinki/kukkuu-ui#693.
@nikomakela nikomakela force-pushed the KK-1436-48h-cancel-limit branch 2 times, most recently from caa17dd to b701695 Compare May 30, 2025 11:18
@nikomakela

Copy link
Copy Markdown
Contributor Author

Screenshots of the cancellation descriptions:

Screenshot 2025-05-30 at 14 00 22 Screenshot 2025-05-30 at 14 13 50 Screenshot 2025-05-30 at 14 14 05 Screenshot 2025-05-30 at 14 14 20

@nikomakela nikomakela force-pushed the KK-1436-48h-cancel-limit branch from b701695 to a20f491 Compare May 30, 2025 11:26
@terovirtanen

Copy link
Copy Markdown
Contributor

KUKKUU-UI branch is deployed to platta: https://kukkuu-ui-pr693.dev.hel.ninja 🚀🚀🚀

@terovirtanen

Copy link
Copy Markdown
Contributor

TestCafe result is failed for https://kukkuu-ui-pr693.dev.hel.ninja 😿💢💥💥

@terovirtanen

Copy link
Copy Markdown
Contributor

KUKKUU-UI branch is deployed to platta: https://kukkuu-ui-pr693.dev.hel.ninja 🚀🚀🚀

@terovirtanen

Copy link
Copy Markdown
Contributor

TestCafe result is failed for https://kukkuu-ui-pr693.dev.hel.ninja 😿💢💥💥

Comment thread src/domain/event/EventIsEnrolled.tsx Outdated
expect(container).toMatchSnapshot();
});

describe('unenrolment is possible only 48 before the occurrence', async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this work because of the ceiling rounding method used in differenceInHours in shouldAllowUnenrolment? I mean that the current time is not frozen here, and as the tests should take less than an hour the difference should evaluate to the same hour value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Now there is 10s cap that should ensure that (and no ceiling rounding).

Comment thread src/domain/event/modal/UnenrolModal.tsx Outdated

@karisal-anders karisal-anders left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The hardcoded 48 values should be changed to use AppConfig.enrolmentCancellationTimeLimitHours except in AppConfig.enrolmentCancellationTimeLimitHours itself as the fallback value.

Maybe a test case for AppConfig.enrolmentCancellationTimeLimitHours returning 48 could be added if the tests are assuming that it must be such for the tests to pass, or the tests could be made so that AppConfig.enrolmentCancellationTimeLimitHours returns 48 in them (e.g. mocking the function's return value or VITE_ENROLMENT_CANCELLATION_TIME_LIMIT_HOURS to 48). Without either of these there is a hidden dependency on the default value of AppConfig.enrolmentCancellationTimeLimitHours being 48 in some of the test cases. Testing for it would at least document the dependency.

And was my assumption of why the tests pass correct related to the use of ceil rounding method in differentInHours i.e. this comment.

I assume Ratkaisutoimisto would want to code review this PR before merging is possible.

@nikomakela nikomakela force-pushed the KK-1436-48h-cancel-limit branch 2 times, most recently from 365d875 to d1d89cc Compare June 2, 2025 08:54
@terovirtanen

Copy link
Copy Markdown
Contributor

KUKKUU-UI branch is deployed to platta: https://kukkuu-ui-pr693.dev.hel.ninja 🚀🚀🚀

@terovirtanen

Copy link
Copy Markdown
Contributor

TestCafe result is success for https://kukkuu-ui-pr693.dev.hel.ninja 😆🎉🎉🎉

@nikomakela nikomakela force-pushed the KK-1436-48h-cancel-limit branch from d1d89cc to e56d8f5 Compare June 2, 2025 09:24
@nikomakela

nikomakela commented Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

The new text in enrolment page was first said that it is not needed, so it's now reverted. But, now I heard that we could use the same text again, what wea re using in other places.
image

@nikomakela nikomakela marked this pull request as ready for review June 2, 2025 09:26
@nikomakela nikomakela requested a review from a team as a code owner June 2, 2025 09:26
@nikomakela nikomakela marked this pull request as draft June 2, 2025 09:31
@terovirtanen

Copy link
Copy Markdown
Contributor

KUKKUU-UI branch is deployed to platta: https://kukkuu-ui-pr693.dev.hel.ninja 🚀🚀🚀

KK-1436.

Add `AppConfig.enrolmentCancellationTimeLimitHours` that can be used to
configure the time (in hours) that limits the cancellation time.

Add description texts related to enrolment cancellation.
@nikomakela nikomakela force-pushed the KK-1436-48h-cancel-limit branch from e56d8f5 to 354a4cc Compare June 2, 2025 09:35
@nikomakela nikomakela marked this pull request as ready for review June 2, 2025 09:35
@sonarqubecloud

sonarqubecloud Bot commented Jun 2, 2025

Copy link
Copy Markdown

@terovirtanen

Copy link
Copy Markdown
Contributor

KUKKUU-UI branch is deployed to platta: https://kukkuu-ui-pr693.dev.hel.ninja 🚀🚀🚀

@terovirtanen

Copy link
Copy Markdown
Contributor

TestCafe result is success for https://kukkuu-ui-pr693.dev.hel.ninja 😆🎉🎉🎉

@nikomakela nikomakela merged commit d1f836c into main Jun 3, 2025
21 checks passed
@nikomakela nikomakela deleted the KK-1436-48h-cancel-limit branch June 3, 2025 08:45
nikomakela added a commit to City-of-Helsinki/kukkuu that referenced this pull request Jun 3, 2025
KK-1436.

Configure `KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE` with environment
variable and use it to limit unenrolments too late before an occurrence
start time. The default value is set to 48h. Use 0 as a value to disable
this new feature.

The UI changes are implemented in
City-of-Helsinki/kukkuu-ui#693.
nikomakela added a commit to City-of-Helsinki/kukkuu that referenced this pull request Jun 3, 2025
KK-1436.

Configure `KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE` with environment
variable and use it to limit unenrolments too late before an occurrence
start time. The default value is set to 48h. Use 0 as a value to disable
this new feature.

The UI changes are implemented in
City-of-Helsinki/kukkuu-ui#693.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Comodori Team Comodori's projected work.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants