mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-06 23:26:55 +01:00
Address review feedback on retention policy knowledge
- Scope both articles to bc-version [17..] (retention policies shipped in v17). - Allowed-tables sample: add OnRefreshAllowedTables subscriber with a ForceUpdate path; the upgrade tag now gates one-time setup only. - Default-policy sample: use Retention Policy Setup.FindOrCreateRetentionPeriod instead of a hand-rolled lookup-then-insert that can collide on code. - Anti-pattern now keys on append-only tables rather than table names. - al-privacy-review: add retention-policy tokens and deterministic routing for both articles, with a bounded per-table text search for delete and registration paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
2f3e5afac1
commit
4bef582ffe
5 changed files with 37 additions and 30 deletions
|
|
@ -37,11 +37,17 @@ Discard files that are not applicable. Retain conditionally applicable files (an
|
|||
|
||||
Narrow the relevant files to the subset that applies to the changes under review. Exclude test codeunits, test libraries, test helper code, files under test/Test/Tests paths, and objects with `Subtype = Test`; test data is synthetic and does not ship to customers. For each relevant file, compute overlap against:
|
||||
|
||||
- The changed AL object names and types — especially tables and tableextensions (for `DataClassification` on fields), codeunits that call `Error`, `Session.LogMessage`, or `FeatureTelemetry`, codeunits performing outgoing HTTP requests with customer data, migration codeunits, and objects reading or writing `IsolatedStorage`.
|
||||
- The changed AL object names and types — especially tables and tableextensions (for `DataClassification` on fields), codeunits that call `Error`, `Session.LogMessage`, or `FeatureTelemetry`, codeunits performing outgoing HTTP requests with customer data, migration codeunits, objects reading or writing `IsolatedStorage`, and install/upgrade codeunits or new tables relevant to retention policies.
|
||||
- The changed procedures and triggers, weighted toward those that call `Error`, construct `ErrorInfo`, call `Session.LogMessage`, `StrSubstNo`, `GetLastErrorText`/`GetLastErrorCallStack`, `FeatureTelemetry.LogUsage`/`LogUptake`/`LogError`, `HttpClient.Post`/`Get`, `IsolatedStorage.Set`/`SetEncrypted`/`Get`, or `PrivacyNotice.GetPrivacyNoticeApprovalState`.
|
||||
- Tokens extracted from the diff that relate to privacy (`DataClassification`, `CustomerContent`, `EndUserIdentifiableInformation`, `EndUserPseudonymousIdentifiers`, `SystemMetadata`, `ToBeClassified`, `PrivacyNotice`, `ErrorInfo`, `GetLastErrorText`, `GetLastErrorCallStack`, `TelemetryScope`, `FeatureTelemetry`, `CustomDimensions`, `LogUsage`, `LogUptake`, `LogError`, `ErrorText`, `ErrorCallStack`, `alErrorText`, `alErrorCallStack`, `HybridSL`, `HybridGP`, `HybridBC`).
|
||||
- Treat `ErrorInfo.Message`, `ErrorInfo.DataClassification`, `ErrorInfo.ErrorType`, and `ErrorInfo.DetailedMessage` as qualified member signals: accept a call or assignment only when symbol resolution proves that its receiver expression or variable has type `ErrorInfo`. Normalize those accesses to `errorinfo-message`, `errorinfo-dataclassification`, `errorinfo-errortype`, and `errorinfo-detailedmessage` retrieval tokens. Bare `Message` or `DataClassification` tokens MUST NOT trigger this article; do not emit the qualified tokens for `Message(...)` dialog calls, table or table-field `DataClassification` properties, or similarly named members on other types. Resolve the receiver's declaration from the containing object when it is outside the changed hunk.
|
||||
- Worklist ErrorInfo privacy guidance only from those typed `ErrorInfo` member tokens or from construction of an `ErrorInfo` value. For every `FeatureTelemetry.LogError`, inspect the dedicated error text and call-stack arguments in addition to explicit custom dimensions.
|
||||
- Retention-policy signals. Emit `addallowedtable` for any call to `Codeunit "Reten. Pol. Allowed Tables".AddAllowedTable`, `retention-policy-setup` for any use of `Record "Retention Policy Setup"`, and `retention-policy` for either. Emit `append-only-table` for an extension-owned table (never a tableextension) that the diff adds, or into which the diff adds a new `Insert` path, when the table also passes this bounded check: search the app's `.al` files for the table's object name once each for `AddAllowedTable`, `Delete`, and `DeleteAll`, and emit the token only when none of the three matches. This is a text search per candidate table — do not read or resolve the rest of the app. Emit `deleteall` when the diff adds a `DeleteAll` on an extension-owned table filtered by a date or datetime field inside a job queue codeunit or other scheduled cleanup and the table has no `AddAllowedTable` call.
|
||||
|
||||
Route retention-policy guidance deterministically:
|
||||
|
||||
- `register-owned-log-tables-for-retention-policies.md` is worklisted by `append-only-table` or `deleteall`, or when an `AddAllowedTable` routine is reached only from an install codeunit, or is guarded by an upgrade tag with no `OnRefreshAllowedTables` subscriber that bypasses the tag. Do not use the table's name as the trigger: an append-only table is a candidate whatever it is called ("Incoming Data" as much as "Activity Log"). A name ending in `Log`, `Entry`, `Archive`, `History`, or `Buffer`, an `AutoIncrement` integer primary key, or inserts from event subscribers, job queue codeunits, or API/web-service handlers raise confidence to `medium`; without any of these the finding stays `low`. Findings from `append-only-table` are advisory (`minor`) because table volume cannot be observed from source.
|
||||
- `ship-a-default-retention-policy-setup.md` is worklisted by `addallowedtable` when the same install or upgrade routine, or the codeunit that contains it, never references `Record "Retention Policy Setup"`. It is also worklisted by `retention-policy-setup` when the diff inserts a setup with `Enabled` validated to true, or builds a `Retention Period` record by hand instead of calling `FindOrCreateRetentionPeriod`.
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Apply the topic-specific gates above after this overlap check; in particular, bare `Message` and `DataClassification` tokens cannot admit ErrorInfo guidance. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue