refactor(audits): use W prefix for warning-level audit error codes - #1733
Conversation
ff44fd1 to
2859a2e
Compare
|
|
||
| ```bash | ||
| rg -n "^\s+[IHMPJVTS]{1,2}_[MIL]_[A-Za-z]+:" \ | ||
| rg -n "^\s+[IHMPJVTS]{1,2}_[MILFW]_[A-Za-z]+:" \ |
There was a problem hiding this comment.
@greenscientist ici le remplacement de rg vers grep est dans #1727
greenscientist
left a comment
greenscientist
left a comment
There was a problem hiding this comment.
Ca me prendrait une explication dans le commit message ou dans un issue sur pourquoi tu ajoute ça
2859a2e to
2752d74
Compare
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
WalkthroughTwo warning-level audit checks were renamed from legacy identifiers to Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ajouté l'explication dans le commit message. On veut pouvoir savoir le type de audit check juste avec son code. |
|
@coderabbitai review |
|
Then my design question is: Why M-L-I for errors, and not just E ?? Kind of does not make sense currently. |
|
@coderabbitai review |
6 similar comments
|
@coderabbitai review |
|
@coderabbitai review |
|
@coderabbitai review |
|
@coderabbitai review |
|
@coderabbitai review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/evolution-backend/src/services/audits/auditChecks/checks/README.md`:
- Around line 25-26: Update the F and W entries in the checks README to use
concise, distinct descriptions: label F as an informational check and W as a
warning that may indicate a possible error, while preserving their existing
examples and meaning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 33861861-d1ad-46af-87d8-fa5eef0e7a19
📒 Files selected for processing (9)
locales/en/audits.jsonlocales/fr/audits.jsonpackages/evolution-backend/src/services/audits/auditChecks/README.mdpackages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.tspackages/evolution-backend/src/services/audits/auditChecks/checks/HouseholdAuditChecks.tspackages/evolution-backend/src/services/audits/auditChecks/checks/README.mdpackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.integration.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/household/HH_W_CarNumberPerPotentialDrivingLicenseTooHigh.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-31T18:20:32.322Z
Learnt from: kaligrafy
Repo: chairemobilite/evolution PR: 1462
File: packages/evolution-common/src/services/baseObjects/Completable.ts:1-6
Timestamp: 2026-03-31T18:20:32.322Z
Learning: In this repository (chairemobilite/evolution), do not flag or require a copyright year in file headers. For new files, the header format `Copyright Polytechnique Montreal and contributors` (without a year) is acceptable. Only verify the header presence/consistency, not the presence of the year.
Applied to files:
packages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/household/HH_W_CarNumberPerPotentialDrivingLicenseTooHigh.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.integration.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.tspackages/evolution-backend/src/services/audits/auditChecks/checks/HouseholdAuditChecks.ts
📚 Learning: 2026-04-21T14:52:21.885Z
Learnt from: kaligrafy
Repo: chairemobilite/evolution PR: 1510
File: packages/evolution-backend/src/services/audits/auditChecks/README.md:148-149
Timestamp: 2026-04-21T14:52:21.885Z
Learning: When adding a new backend audit check error code in the Evolution project, you must also add the corresponding translations in locales/en/audits.json and locales/fr/audits.json. This is required because the UI looks up error codes via t('audits:${errorCode}') and the CI test AuditLocales.test.ts enforces that all audit error codes have locale entries; without the locale keys, the build should fail.
Applied to files:
locales/en/audits.jsonlocales/fr/audits.json
🔇 Additional comments (8)
packages/evolution-backend/src/services/audits/auditChecks/README.md (1)
114-128: LGTM!Also applies to: 307-308, 320-320
packages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.ts (1)
111-128: LGTM!locales/en/audits.json (1)
16-16: LGTM!Also applies to: 25-25
locales/fr/audits.json (1)
16-16: LGTM!Also applies to: 25-25
packages/evolution-backend/src/services/audits/auditChecks/checks/HouseholdAuditChecks.ts (1)
239-239: LGTM!Also applies to: 262-262
packages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.test.ts (1)
18-18: LGTM!Also applies to: 59-59, 94-94, 129-136
packages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.integration.test.ts (1)
12-12: LGTM!Also applies to: 32-36, 60-60, 82-82
packages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/household/HH_W_CarNumberPerPotentialDrivingLicenseTooHigh.test.ts (1)
15-15: LGTM!Also applies to: 77-83
| - **Interview** (`InterviewAuditChecks.ts`): `I_M_Languages`, `I_M_StartedAt`, `I_I_StartedAtBeforeSurveyStartDate`, `I_I_StartedAtAfterSurveyEndDate`, `I_M_AccessCode`, `I_I_InvalidAccessCodeFormat`, `I_I_ContactEmail`, `I_M_ContactEmailButWouldLikeToParticipateInOtherSurveys`, `I_I_HelpContactEmail`. | ||
| - **Household**: `HH_M_Size`, `HH_I_Size`, `HH_M_Home`, `HH_L_SizeMembersCountMismatch`, `HH_M_CarNumber`, `HH_I_CarNumber`, `HH_L_CarNumberVehiclesCountMismatch`. | ||
| - **Home**: `HM_M_Geography`, `HM_I_Geography`, `HM_I_preGeographyAndHomeGeographyTooFarApartError`, `HM_I_preGeographyAndHomeGeographyTooFarApartWarning`, `HM_I_geographyNotInSurveyTerritory`. | ||
| - **Household**: `HH_M_Size`, `HH_I_Size`, `HH_M_Home`, `HH_L_SizeMembersCountMismatch`, `HH_M_CarNumber`, `HH_I_CarNumber`, `HH_W_CarNumberPerPotentialDrivingLicenseTooHigh`, `HH_L_CarNumberVehiclesCountMismatch`, `HH_F_AtLeastOneTransitSegmentInHousehold`. |
There was a problem hiding this comment.
Cette liste n'a pas été enlevée dans un autre commit? Il faudrait rebaser probablement
THis was like this before: M: missing something now, I just want to add W for any warning (was missing in 2023) and document F for Info (like household has at least one transit trip) MILF were used before in 2023, so chaning this needs more discussion because then audit naming would not be comparable between the years. I would suggest making the change and you can create a separate issue for renaming/refactoring the audit codes |
|
Waiting for #1744 and fixing conflict then. |
0a0d3b7 to
4130bac
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.ts (1)
111-125: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve backward compatibility for persisted audit codes.
The rename changes ignored-audit matching and removes translation keys used by existing records. Without a migration or aliases, old audits can reappear as unignored and display untranslated messages.
packages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.ts#L111-L125: reconcileHM_I_preGeographyAndHomeGeographyTooFarApartWarningwith the newHM_W_code.packages/evolution-backend/src/services/audits/auditChecks/checks/HouseholdAuditChecks.ts#L260-L283: reconcileHH_L_CarNumberPerPotentialDrivingLicenseTooHighwith the newHH_W_code.locales/en/audits.json#L16-L26: retain English aliases for both legacy codes.locales/fr/audits.json#L16-L26: retain French aliases for both legacy codes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.ts` around lines 111 - 125, Preserve persisted audit-code compatibility: in HomeAuditChecks.ts, reconcile legacy HM_I_preGeographyAndHomeGeographyTooFarApartWarning with HM_W_preGeographyAndHomeGeographyTooFarApart for ignored-audit matching; in HouseholdAuditChecks.ts, likewise reconcile HH_L_CarNumberPerPotentialDrivingLicenseTooHigh with its new HH_W_ code. Add English aliases in locales/en/audits.json and French aliases in locales/fr/audits.json for both legacy codes.Source: Learnings
♻️ Duplicate comments (1)
packages/evolution-backend/src/services/audits/auditChecks/checks/README.md (1)
25-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClean up the duplicated F/W wording.
Line [25] says “Info info” and Line [26] says “Warning warning”. Use concise descriptions such as “Informational check” and “Warning that may indicate a possible error.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evolution-backend/src/services/audits/auditChecks/checks/README.md` around lines 25 - 26, Update the README entries for the F and W audit check codes to remove the duplicated “Info info” and “Warning warning” wording, using concise descriptions such as “Informational check” and “Warning that may indicate a possible error.”
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/evolution-backend/src/services/audits/auditChecks/README.md`:
- Line 300: Update the inventory grep regex in the README command to use
`[MILFW]` for the middle prefix character, including `_F_` checks and excluding
the invalid `_N_` prefix while preserving the other matching behavior.
---
Outside diff comments:
In
`@packages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.ts`:
- Around line 111-125: Preserve persisted audit-code compatibility: in
HomeAuditChecks.ts, reconcile legacy
HM_I_preGeographyAndHomeGeographyTooFarApartWarning with
HM_W_preGeographyAndHomeGeographyTooFarApart for ignored-audit matching; in
HouseholdAuditChecks.ts, likewise reconcile
HH_L_CarNumberPerPotentialDrivingLicenseTooHigh with its new HH_W_ code. Add
English aliases in locales/en/audits.json and French aliases in
locales/fr/audits.json for both legacy codes.
---
Duplicate comments:
In `@packages/evolution-backend/src/services/audits/auditChecks/checks/README.md`:
- Around line 25-26: Update the README entries for the F and W audit check codes
to remove the duplicated “Info info” and “Warning warning” wording, using
concise descriptions such as “Informational check” and “Warning that may
indicate a possible error.”
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f0a9cc7-32a8-4004-b5c2-db64f41099de
📒 Files selected for processing (9)
locales/en/audits.jsonlocales/fr/audits.jsonpackages/evolution-backend/src/services/audits/auditChecks/README.mdpackages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.tspackages/evolution-backend/src/services/audits/auditChecks/checks/HouseholdAuditChecks.tspackages/evolution-backend/src/services/audits/auditChecks/checks/README.mdpackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.integration.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/household/HH_W_CarNumberPerPotentialDrivingLicenseTooHigh.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-31T18:20:32.322Z
Learnt from: kaligrafy
Repo: chairemobilite/evolution PR: 1462
File: packages/evolution-common/src/services/baseObjects/Completable.ts:1-6
Timestamp: 2026-03-31T18:20:32.322Z
Learning: In this repository (chairemobilite/evolution), do not flag or require a copyright year in file headers. For new files, the header format `Copyright Polytechnique Montreal and contributors` (without a year) is acceptable. Only verify the header presence/consistency, not the presence of the year.
Applied to files:
packages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.integration.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/household/HH_W_CarNumberPerPotentialDrivingLicenseTooHigh.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/HouseholdAuditChecks.tspackages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.ts
📚 Learning: 2026-04-21T14:52:21.885Z
Learnt from: kaligrafy
Repo: chairemobilite/evolution PR: 1510
File: packages/evolution-backend/src/services/audits/auditChecks/README.md:148-149
Timestamp: 2026-04-21T14:52:21.885Z
Learning: When adding a new backend audit check error code in the Evolution project, you must also add the corresponding translations in locales/en/audits.json and locales/fr/audits.json. This is required because the UI looks up error codes via t('audits:${errorCode}') and the CI test AuditLocales.test.ts enforces that all audit error codes have locale entries; without the locale keys, the build should fail.
Applied to files:
locales/en/audits.jsonlocales/fr/audits.json
🔇 Additional comments (5)
packages/evolution-backend/src/services/audits/auditChecks/README.md (1)
114-128: LGTM!Also applies to: 147-161
packages/evolution-backend/src/services/audits/auditChecks/checks/HouseholdAuditChecks.ts (1)
10-13: LGTM!Also applies to: 176-218
packages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.test.ts (1)
18-18: LGTM!Also applies to: 59-59, 94-94, 129-134
packages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.integration.test.ts (1)
12-12: LGTM!Also applies to: 32-36, 60-60, 82-82
packages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/household/HH_W_CarNumberPerPotentialDrivingLicenseTooHigh.test.ts (1)
15-15: LGTM!Also applies to: 77-83
|
|
||
| ```bash | ||
| grep -rEn "^[[:space:]]+[IHMPJVTS]{1,2}_[MIL]_[A-Za-z]+:" \ | ||
| grep -rEn "^[[:space:]]+[IHMPJVTS]{1,2}_[MILWN]_[A-Za-z]+:" \ |
| - `L` - **Logical** error (e.g., `J_L_EndBeforeStart` - journey end time before start time) | ||
| - `W` - **Warning**: suspicious data that is still usable, worth a reviewer's attention (e.g., `HH_W_PersonSequenceGaps`). Use with `level: 'warning'`. | ||
| - `F` - **Info** info, not related to any error or warning: ex: "HH_F_AtLeastOneTransitSegmentInHousehold" | ||
| - `W` - **Warning** warning, may be a possible error, needs review/approval |
There was a problem hiding this comment.
Le hint "Use with level: 'warning' demeure pertinent dans la doc.
4130bac to
f575367
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.ts (1)
111-125: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd compatibility handling for changed audit identifiers.
These checks now emit
HM_W_preGeographyAndHomeGeographyTooFarApartandHH_W_CarNumberPerPotentialDrivingLicenseTooHighonly. Existing ignored audits with the former identifiers will not be matched bymergeWithExisting, and the new warning for the home geography distance would lack a French translation. Add a migration/compatibility layer for ignored legacy IDs, or bump the check version and translate the legacy code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.ts` around lines 111 - 125, The audit identifier changes in HomeAuditChecks and HouseholdAuditChecks need backward compatibility, and the new home-geography warning needs localization. Update the ignored-audit matching or migration logic so legacy identifiers still match mergeWithExisting, and add the corresponding translations for HM_W_preGeographyAndHomeGeographyTooFarApart in locales/en/audits.json and locales/fr/audits.json; apply the equivalent identifier handling for HH_W_CarNumberPerPotentialDrivingLicenseTooHigh.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.ts`:
- Around line 111-125: The audit identifier changes in HomeAuditChecks and
HouseholdAuditChecks need backward compatibility, and the new home-geography
warning needs localization. Update the ignored-audit matching or migration logic
so legacy identifiers still match mergeWithExisting, and add the corresponding
translations for HM_W_preGeographyAndHomeGeographyTooFarApart in
locales/en/audits.json and locales/fr/audits.json; apply the equivalent
identifier handling for HH_W_CarNumberPerPotentialDrivingLicenseTooHigh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4aa36176-7961-4833-957d-c9422be407ef
📒 Files selected for processing (9)
locales/en/audits.jsonlocales/fr/audits.jsonpackages/evolution-backend/src/services/audits/auditChecks/README.mdpackages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.tspackages/evolution-backend/src/services/audits/auditChecks/checks/HouseholdAuditChecks.tspackages/evolution-backend/src/services/audits/auditChecks/checks/README.mdpackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.integration.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/household/HH_W_CarNumberPerPotentialDrivingLicenseTooHigh.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-31T18:20:32.322Z
Learnt from: kaligrafy
Repo: chairemobilite/evolution PR: 1462
File: packages/evolution-common/src/services/baseObjects/Completable.ts:1-6
Timestamp: 2026-03-31T18:20:32.322Z
Learning: In this repository (chairemobilite/evolution), do not flag or require a copyright year in file headers. For new files, the header format `Copyright Polytechnique Montreal and contributors` (without a year) is acceptable. Only verify the header presence/consistency, not the presence of the year.
Applied to files:
packages/evolution-backend/src/services/audits/auditChecks/checks/HouseholdAuditChecks.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/household/HH_W_CarNumberPerPotentialDrivingLicenseTooHigh.test.tspackages/evolution-backend/src/services/audits/auditChecks/checks/HomeAuditChecks.tspackages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.integration.test.ts
📚 Learning: 2026-04-21T14:52:21.885Z
Learnt from: kaligrafy
Repo: chairemobilite/evolution PR: 1510
File: packages/evolution-backend/src/services/audits/auditChecks/README.md:148-149
Timestamp: 2026-04-21T14:52:21.885Z
Learning: When adding a new backend audit check error code in the Evolution project, you must also add the corresponding translations in locales/en/audits.json and locales/fr/audits.json. This is required because the UI looks up error codes via t('audits:${errorCode}') and the CI test AuditLocales.test.ts enforces that all audit error codes have locale entries; without the locale keys, the build should fail.
Applied to files:
locales/en/audits.jsonlocales/fr/audits.json
🔇 Additional comments (5)
packages/evolution-backend/src/services/audits/auditChecks/README.md (1)
114-128: LGTM!Also applies to: 300-300
packages/evolution-backend/src/services/audits/auditChecks/checks/README.md (1)
25-26: LGTM!packages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.test.ts (1)
18-18: LGTM!Also applies to: 59-59, 94-94, 129-134
packages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/home/HM_W_preGeographyAndHomeGeographyTooFarApart.integration.test.ts (1)
12-12: LGTM!Also applies to: 32-36, 60-60, 82-82
packages/evolution-backend/src/services/audits/auditChecks/checks/__tests__/household/HH_W_CarNumberPerPotentialDrivingLicenseTooHigh.test.ts (1)
15-15: LGTM!Also applies to: 77-83
f575367 to
4057f0d
Compare
So maybe You should have a 2 letter code? One letter for the level and one for the type [WRI][MIL] It would make much more sense. |
greenscientist
left a comment
greenscientist
left a comment
There was a problem hiding this comment.
It just feel that this whole thing need an actual design effort and we are just patching things on top of it without a deep reflexion. You should minimially create an high pri issue to revise this system...
| - `_I_` — **Invalid** data (`HH_I_Size` — out of range, malformed, wrong type). | ||
| - `_L_` — **Logical** inconsistency between fields or objects (`HH_L_SizeMembersCountMismatch`). | ||
| - `_F_` — **Info** — informational flag, not an error or warning (`HH_F_AtLeastOneTransitSegmentInHousehold`). | ||
| - `_W_` — **Warning** — suspicious data that may be fine but needs reviewer attention (`HH_W_CarNumberPerPotentialDrivingLicenseTooHigh`, `HM_W_preGeographyAndHomeGeographyTooFarApart`). |
There was a problem hiding this comment.
Should we have a type specific for NEED_REVIEW? This feel different than a warning, where a warning is just a notice that can be investigate at some point but will not affect results??
There was a problem hiding this comment.
True, I will propose something more stable and better. Back in few minutes (hours?)
greenscientist
left a comment
greenscientist
left a comment
There was a problem hiding this comment.
Accepted, but please add the asked comment before merging.
| ### 3.2 What kind of problem is it? | ||
|
|
||
| Follow the existing prefix convention (see the checks folder README): | ||
|
|
There was a problem hiding this comment.
Last thing: Can you add a kind of TODO comment here, mentionning that the following mix up type and severity. and refer to the issue that you just created there, so that when someone check that and see that it's weird, they know why.
| | `warning` | Something suspicious that often turns out fine. | Should look at it when they have time. | | ||
| | `info` | Informational only, typically for stats or routing. | Can ignore. | | ||
| | Level | Prefix | Meaning | Reviewer behavior expected | | ||
| | --------- | ------ | -------------------------------------------------------------------------- | -------------------------------------- | |
There was a problem hiding this comment.
Maybe also put a similar comment here mentionning the same.
Just having the error code makes it possible to know the audit check type. Audit error codes should encode audit check type in the prefix: M/I/L for errors, W for warnings, F for info. Two existing checks used L or I despite level: 'warning'; rename them and document F/W in the README. Code written by Cursor Composer 2.5 and reviewed by @kaligrafy
4057f0d to
f222bda
Compare
3 participants
Summary
HH_L_CarNumberPerPotentialDrivingLicenseTooHigh→HH_W_CarNumberPerPotentialDrivingLicenseTooHighHM_I_preGeographyAndHomeGeographyTooFarApartWarning→HM_W_preGeographyAndHomeGeographyTooFarApartTest plan
yarn test --testPathPatterns="HH_W_|HM_W_|AuditLocales"inpackages/evolution-backendNote
Existing stored audits use the old error codes; re-running audits will produce the new codes. Previously ignored audits under old codes will not carry over automatically.
Summary by CodeRabbit
_F_and_W_codes._W_audit identifiers and verify expected warning-level outputs.