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 - forbidden_real_time_booking_field_value validation notice #1845

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

cka-y
Copy link
Contributor

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

Summary:

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

  • booking_type = 0 (Real-time booking)
  • Any of the following fields are present in the booking rule:
    • prior_notice_duration_min
    • prior_notice_duration_max
    • prior_notice_last_day
    • prior_notice_last_time
    • prior_notice_start_day
    • prior_notice_start_time
    • prior_notice_service_id

Expected Behavior:

A validation notice is generated if the above conditions are met, flagging the presence of forbidden fields in a real-time booking rule.

Screenshot:
image

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 23, 2024 that may be closed by this pull request
@emmambd
Copy link
Contributor

emmambd commented Sep 23, 2024

@tzujenchanmbd @Sergiodero Since these booking rule changes are happening faster than I expected (thanks @cka-y!) How about we split up the QA responsibility.

  • I can be responsible for actually running the test feeds to see if they generate as expected
  • You are only expected to provide feedback on the notice name, description, and columns displayed in the screenshot.

Seem reasonable?

Copy link
Contributor

📝 Acceptance Test Report

📋 Summary

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

📊 Notices Comparison

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

No changes were detected due to the code change.

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.07 4.11 ⬆️+0.04
Median -- 1.41 1.45 ⬆️+0.04
Standard Deviation -- 11.63 11.61 ⬇️-0.02
Minimum in References Reports us-california-flex-v2-developer-test-feed-2-gtfs-1818 0.49 0.54 ⬆️+0.05
Maximum in Reference Reports gb-unknown-uk-aggregate-feed-gtfs-2014 298.52 296.12 ⬇️-2.40
Minimum in Latest Reports us-florida-citrus-county-transit-gtfs-630 0.55 0.52 ⬇️-0.03
Maximum in Latest Reports gb-unknown-uk-aggregate-feed-gtfs-2014 298.52 296.12 ⬇️-2.40

@tzujenchanmbd
Copy link

tzujenchanmbd commented Sep 24, 2024

Based on the definition of Field and Field value in GTFS - https://gtfs.org/documentation/schedule/reference/#term-definitions, suggesting the following changes:

Notice name: forbidden_real_time_booking_field_value
Description:
"A forbidden field value is present for a real-time booking rule.
You can see more about this notice here."

Columns displayed lgtm!

@Sergiodero
Copy link

LGTM as well, I just have one suggestion: I'm not sure if this will align well with other error descriptions in the validator, but I think it might be useful to explicitly point out the fact that the error is originated in the booking_rules.txt file. Feel free to ignore this if you're planning on stating this in more detail in the Validator rules

@emmambd
Copy link
Contributor

emmambd commented Sep 24, 2024

@cka-y Yes to @Sergiodero's point - maybe we could say: "A forbidden field value is present for a real-time booking rule in booking_rules.txt.
You can see more about this notice here."

@emmambd emmambd changed the title feat: flex - forbidden_real_time_booking_field validation notice feat: flex - forbidden_real_time_booking_field_value validation notice Sep 24, 2024
@cka-y
Copy link
Contributor Author

cka-y commented Sep 24, 2024

✅ Done @Sergiodero @tzujenchanmbd
I updated the screenshot in the issue description to reflect the changes

@emmambd
Copy link
Contributor

emmambd commented Sep 24, 2024

Test data works as expected :) QA side ✅

Copy link
Contributor

📝 Acceptance Test Report

📋 Summary

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

📊 Notices Comparison

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

No changes were detected due to the code change.

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.07 4.11 ⬆️+0.04
Median -- 1.44 1.47 ⬆️+0.03
Standard Deviation -- 11.61 11.54 ⬇️-0.07
Minimum in References Reports us-florida-citrus-county-transit-gtfs-630 0.51 0.51 ⬆️+0.01
Maximum in Reference Reports gb-unknown-uk-aggregate-feed-gtfs-2014 295.89 292.09 ⬇️-3.80
Minimum in Latest Reports us-florida-citrus-county-transit-gtfs-630 0.51 0.51 ⬆️+0.01
Maximum in Latest Reports gb-unknown-uk-aggregate-feed-gtfs-2014 295.89 292.09 ⬇️-3.80

Copy link
Contributor

@jcpitre jcpitre left a comment

Choose a reason for hiding this comment

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

LGTM

@cka-y cka-y merged commit 9b60fff into master Sep 24, 2024
333 checks passed
@cka-y cka-y deleted the feat/1824 branch September 24, 2024 22:48
Copy link
Contributor

📝 Acceptance Test Report

📋 Summary

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

📊 Notices Comparison

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

No changes were detected due to the code change.

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.06 4.12 ⬆️+0.06
Median -- 1.43 1.47 ⬆️+0.04
Standard Deviation -- 11.53 11.57 ⬆️+0.04
Minimum in References Reports us-massachusetts-massachusetts-area-express-max-gtfs-431 0.54 0.53 ⬇️-0.01
Maximum in Reference Reports gb-unknown-uk-aggregate-feed-gtfs-2014 289.59 288.56 ⬇️-1.02
Minimum in Latest Reports us-california-flex-v2-developer-test-feed-3-gtfs-1819 0.57 0.52 ⬇️-0.05
Maximum in Latest Reports gb-unknown-uk-aggregate-feed-gtfs-2014 289.59 288.56 ⬇️-1.02

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_real_time_booking_field
5 participants