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

feat: flex - added forbidden_same_day_booking_field_value notice #1847

Merged
merged 6 commits into from
Oct 1, 2024

Conversation

cka-y
Copy link
Contributor

@cka-y cka-y commented Sep 25, 2024

Summary

This PR introduces a new validation rule that triggers an ERROR severity notice when the following conditions are met:

  • booking_type = 1 (Same-day booking)
  • Any of the following fields are present in the booking rule:
    • prior_notice_last_day
    • prior_notice_last_time
    • prior_notice_start_time
    • prior_notice_service_id

Expected Behavior

A validation notice will be generated if the conditions listed above are met, flagging the presence of restricted fields in a same-day booking rule.

Example using this test dataset.

Screenshot 2024-09-25 at 9 41 42 AM

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with gradle test to make sure you didn't break anything
  • Add or update any needed documentation to the repo
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@cka-y cka-y linked an issue Sep 25, 2024 that may be closed by this pull request
Copy link
Contributor

📝 Acceptance Test Report

📋 Summary

✅ The rule acceptance has passed for commit 5c3dab2
Download the full acceptance test report here (report will disappear after 90 days).

📊 Notices Comparison

New Errors (1 out of 1575 datasets, ~0%) ✅

Details of new errors due to code change, which is less than the provided threshold of 1%.

Dataset Notice Code
us-colorado-gunnison-valley-rta-gtfs-2048 forbidden_same_day_booking_field_value
Dropped Errors (0 out of 1575 datasets, ~0%) ✅

No changes were detected due to the code change.

New Warnings (0 out of 1575 datasets, ~0%) ✅

No changes were detected due to the code change.

Dropped Warnings (0 out of 1575 datasets, ~0%) ✅

No changes were detected due to the code change.

🛡️ Corruption Check

0 out of 1575 sources (~0 %) are corrupted.

⏱️ Performance Assessment

📈 Validation Time

Assess the performance in terms of seconds taken for the validation process.

Time Metric Dataset ID Reference (s) Latest (s) Difference (s)
Average -- 4.09 4.19 ⬆️+0.10
Median -- 1.43 1.47 ⬆️+0.04
Standard Deviation -- 11.73 12.47 ⬆️+0.75
Minimum in References Reports tr-kocaeli-metro-izmir-gtfs-1824 0.52 0.62 ⬆️+0.10
Maximum in Reference Reports gb-unknown-uk-aggregate-feed-gtfs-2014 301.08 344.96 ⬆️+43.88
Minimum in Latest Reports us-california-city-of-wasco-gtfs-1788 0.56 0.49 ⬇️-0.07
Maximum in Latest Reports gb-unknown-uk-aggregate-feed-gtfs-2014 301.08 344.96 ⬆️+43.88

@emmambd
Copy link
Contributor

emmambd commented Sep 25, 2024

Yay for acceptance tests! @tzujenchanmbd After taking a look at the new error, I think we missed an exception in the spec:
Screenshot 2024-09-25 at 12 25 04 PM

report.json The report shows that prior_notice_start_time is a forbidden field for same day booking. However, it's required in cases where prior_notice_start_day is defined`.

prior_notice_start day is only Forbidden for booking_type=1 if prior_notice_duration_max is defined. In the feed above, prior_notice_duration_max does not exist and prior_notice_start_day is defined, so it should not trigger the error.

I think the fix here is to just remove prior_notice_start_time as a forbidden field and have #1827 and #1832 manage this use case.

Can you confirm before @cka-y updates the logic?

@cka-y
Copy link
Contributor Author

cka-y commented Sep 25, 2024

seems like the logic you're mentioning @emmambd is also referenced in #1827

@tzujenchanmbd
Copy link

I think the fix here is to just remove prior_notice_start_time as a forbidden field and have #1827 and #1832 manage this use case.

LGTM. It seems #1827, #1832, and #1833 can handle prior_notice_start_time well. We can remove it here.

Notice name, description, and columns displayed also lgtm!

Copy link
Contributor

@qcdyx qcdyx left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

📝 Acceptance Test Report

📋 Summary

✅ The rule acceptance has passed for commit 0c9b0f3
Download the full acceptance test report here (report will disappear after 90 days).

📊 Notices Comparison

New Errors (1 out of 1588 datasets, ~0%) ✅

Details of new errors due to code change, which is less than the provided threshold of 1%.

Dataset Notice Code
us-colorado-gunnison-valley-rta-gtfs-2048 forbidden_same_day_booking_field_value
Dropped Errors (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

New Warnings (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

Dropped Warnings (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

🛡️ Corruption Check

0 out of 1588 sources (~0 %) are corrupted.

⏱️ Performance Assessment

📈 Validation Time

Assess the performance in terms of seconds taken for the validation process.

Time Metric Dataset ID Reference (s) Latest (s) Difference (s)
Average -- 4.07 4.22 ⬆️+0.15
Median -- 1.43 1.53 ⬆️+0.10
Standard Deviation -- 11.56 11.76 ⬆️+0.20
Minimum in References Reports us-california-flex-v2-developer-test-feed-2-gtfs-1818 0.50 0.51 ⬆️+0.01
Maximum in Reference Reports gb-unknown-uk-aggregate-feed-gtfs-2014 296.40 301.68 ⬆️+5.27
Minimum in Latest Reports us-california-flex-v2-developer-test-feed-2-gtfs-1818 0.50 0.51 ⬆️+0.01
Maximum in Latest Reports gb-unknown-uk-aggregate-feed-gtfs-2014 296.40 301.68 ⬆️+5.27

@emmambd
Copy link
Contributor

emmambd commented Sep 30, 2024

Looks like this has been approved twice - just want to be explicit the logic change has to be made above to remove prior_notice_start_time as a forbidden field before this should be merged.

Copy link
Contributor

📝 Acceptance Test Report

📋 Summary

✅ The rule acceptance has passed for commit f76c755
Download the full acceptance test report here (report will disappear after 90 days).

📊 Notices Comparison

New Errors (1 out of 1588 datasets, ~0%) ✅

Details of new errors due to code change, which is less than the provided threshold of 1%.

Dataset Notice Code
us-colorado-gunnison-valley-rta-gtfs-2048 forbidden_same_day_booking_field_value
Dropped Errors (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

New Warnings (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

Dropped Warnings (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

🛡️ Corruption Check

0 out of 1588 sources (~0 %) are corrupted.

⏱️ Performance Assessment

📈 Validation Time

Assess the performance in terms of seconds taken for the validation process.

Time Metric Dataset ID Reference (s) Latest (s) Difference (s)
Average -- 4.09 4.18 ⬆️+0.09
Median -- 1.44 1.50 ⬆️+0.06
Standard Deviation -- 11.70 11.70 ⬆️+0.01
Minimum in References Reports us-california-flex-v2-developer-test-feed-2-gtfs-1818 0.50 0.60 ⬆️+0.10
Maximum in Reference Reports gb-unknown-uk-aggregate-feed-gtfs-2014 302.12 297.35 ⬇️-4.77
Minimum in Latest Reports us-oregon-hut-airport-shuttle-gtfs-635 0.54 0.58 ⬆️+0.04
Maximum in Latest Reports gb-unknown-uk-aggregate-feed-gtfs-2014 302.12 297.35 ⬇️-4.77

Copy link
Contributor

github-actions bot commented Oct 1, 2024

📝 Acceptance Test Report

📋 Summary

✅ The rule acceptance has passed for commit b83dbea
Download the full acceptance test report here (report will disappear after 90 days).

📊 Notices Comparison

New Errors (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

Dropped Errors (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

New Warnings (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

Dropped Warnings (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

🛡️ Corruption Check

0 out of 1588 sources (~0 %) are corrupted.

⏱️ Performance Assessment

📈 Validation Time

Assess the performance in terms of seconds taken for the validation process.

Time Metric Dataset ID Reference (s) Latest (s) Difference (s)
Average -- 4.09 4.15 ⬆️+0.06
Median -- 1.43 1.49 ⬆️+0.06
Standard Deviation -- 11.63 11.54 ⬇️-0.09
Minimum in References Reports us-california-flex-v2-developer-test-feed-2-gtfs-1818 0.51 0.52 ⬆️+0.01
Maximum in Reference Reports gb-unknown-uk-aggregate-feed-gtfs-2014 290.11 291.82 ⬆️+1.71
Minimum in Latest Reports us-california-flex-v2-developer-test-feed-2-gtfs-1818 0.51 0.52 ⬆️+0.01
Maximum in Latest Reports gb-unknown-uk-aggregate-feed-gtfs-2014 290.11 291.82 ⬆️+1.71

@cka-y
Copy link
Contributor Author

cka-y commented Oct 1, 2024

it has now been addressed @emmambd

@cka-y cka-y merged commit ed02ed1 into master Oct 1, 2024
335 checks passed
@cka-y cka-y deleted the feat/1825 branch October 1, 2024 15:15
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.

Flex: forbidden_same_day_booking_field_value
5 participants