Skip to content

[Master Data Management] Fixing synchronization of contacts, media, date, datetime, source watermarking, permission issue and minor UX issues - #11626

Open
dcenic wants to merge 5 commits into
mainfrom
bugs/650543MDMMediaSynchFix
Open

dcenic wants to merge 5 commits into
mainfrom
bugs/650543MDMMediaSynchFix

Conversation

@dcenic

@dcenic dcenic commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

What & why

  • Media/watermark cache lifetime fix — resets only at the batch boundary (GetModifiedBatch); ancillary re-fetches no longer wipe in-flight batch data. Guarded by the three new tests.
  • When admin mises the consent of Privacy Notice on source environment, show actionable message instead of the the bare 403 response error.
  • Fixing contact synchronization issue (Contact No. already taken by a contact of type company that was auto-created with customer/vendor creation)
  • Fixing synchronization of blanked date and datetime values.
  • Fixing missing indirect permission issues that non-SUPER users face.
  • Fixing minor UX issues on setup page and integration synchronization errors page.

Linked work

Fixes AB#650543 AB#650539 AB#650738 AB#650747 AB#647736 AB#648540 AB#650735

How I validated this

  • I read the full diff and it contains only changes I intended.
  • I built the affected app(s) locally with no new analyzer warnings.
  • I ran the change in Business Central and confirmed it behaves as expected.
  • I added or updated tests for the new behavior, or explained below why none are needed.

What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)

Risk & compatibility

Low risk, and must fix.

…arking and error message when admin didn't consent privacy notice
@dcenic
dcenic requested a review from a team as a code owner September 18, 2026 14:26
@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 Team: Integrations GitHub request for Integrations area labels Sep 18, 2026
@github-actions github-actions Bot added this to the Version 30.0 milestone Sep 18, 2026
@alexei-dobriansky

This comment was marked as resolved.

@dcenic

dcenic commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Good Sense Reviewer - Round 1

Recommendation: Request Changes

What this PR does
This changes cross-environment master data synchronization so per-batch inline media and source watermark caches survive the lookup and relation reads that happen during the write loop. It also lets the source app read the privacy notice state and gives a clearer setup message for 401/403 source access failures.

The batch-boundary reset is the right direction for the picture and watermark bugs, and the added tests cover the main cache-wipe paths. One cache state transition is still unsafe: if the same source record is re-read in the same batch after its media state changes, old per-record media state can still win during transfer.

Problem-solution fit
Fit: Partial

The change targets the reported cache lifetime bug and the unclear source access error. It covers the common paths, but the media cache now needs per-record state replacement so a later re-fetch cannot use stale media state for the same source record.

Suggestions
S1 (🔴 High): Clear stale per-record media state A re-fetch can read the same source record again after its picture changed. MDM Inline Media still keeps the old ClearedByKey or content entry, and UpdateMediaCrossEnvironment can clear or copy the wrong picture. Clear the old per-record media state before storing the new content, cleared, or skipped result.

Risk assessment and necessity
Risk: The main regression surface is cross-environment Master Data Management synchronization for media fields and source timestamps. The permission change expands the source app's read access only to the privacy notice tables, and there is no BaseApp event dependency or public API signature change.

Necessity: The change is needed because ancillary reads during a sync run must not erase media bytes or source watermarks that the write loop still needs. The scope is focused, but the stale per-record media state must be fixed before the cache lifetime is extended safely.

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11626 round=1 by=alexei-dobriansky at=2026-09-18T22:21:47.8561288Z lastSha=b0d410b73b1f6853beaead28f897ee4da1464b6c reviewKey=44f2e3451ddfbeefa0bbd6b609992748eb5e82e1cd074ae93e18929e0b3c8bca suggestions=S1@c17d1766

Thanks — the stale per-record state is already cleared, just at the batch boundary rather than per-Put.

Every sync run enters through GetModifiedBatch, which calls InlineMedia.Reset() before materializing the batch, and Reset() clears all four maps together — ContentByKey, NameByKey, MimeByKey, and ClearedByKey. So no per-record state (content or cleared) survives from one batch/run into the next.

Within a single batch, a given (SystemId, fieldNo) key can't hold a conflicting content-vs-cleared pair:

A media change bumps the source record's SystemModifiedAt, so the record resurfaces in a later detector run — a fresh GetModifiedBatch → Reset() — not as a mid-batch state flip.
The same-batch ancillary re-read (GetBySystemId/GetById/GetByUidFilter, or the relation reads) hits the same committed source record, so ApplyInlineMedia takes the same branch again and re-writes the identical result for that key (content stays content, cleared stays cleared). It never flips one to the other for a live key.
So UpdateMediaCrossEnvironment's IsCleared-then-TryGet order can't pick up a stale entry: the only inputs for a key are a consistent snapshot of that record's current media, and cross-batch staleness is handled by the boundary Reset(). The scenario in S1 (same record re-read after its picture changed, inside the same batch) would require the source to mutate mid-run and be re-read within that same run — outside the snapshot the sync already relies on (same as same-env sync).

Therefore, the decision is to do no code change related to suggestion S1.

@dcenic dcenic changed the title [Master Data Management] Fixing the inline media synch, source watermarking and error message when admin didn't consent privacy notice [Master Data Management] Fixing the inline media synch, source watermarking, contact synch and minor UX issues Sep 21, 2026
…atetime and re-adding tests that were mistakenly dropped during PR creation
@dcenic dcenic changed the title [Master Data Management] Fixing the inline media synch, source watermarking, contact synch and minor UX issues [Master Data Management] Fixing synchronization of contacts, media, date, datetime, source watermarking and minor UX issues Sep 21, 2026
@dcenic dcenic changed the title [Master Data Management] Fixing synchronization of contacts, media, date, datetime, source watermarking and minor UX issues [Master Data Management] Fixing synchronization of contacts, media, date, datetime, source watermarking, permission issue and minor UX issues Sep 21, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Request Changes

What this PR does

The new commits add fixes for blank Date/DateTime transfer, duplicate coupling inserts, deletion-conflict messages, page permissions and visibility, and tests for those paths. These changes look focused and fit the affected Master Data Management sync flows.

The previous media-cache issue is still present in the current net diff. The new commits do not change the per-record inline media replacement path, so the cache can still keep stale state for the same source record.

Status of previous suggestions
ID Title Status Author response
S1 Clear stale per-record media state Not addressed No reply found; the new commits do not change the per-record media cache replacement path.
New observations (commits since round 1)

None - the new commits add focused fixes and tests, and I found no new blocking issue in those changed spans.

Risk assessment and necessity

Risk: The regression surface is cross-environment Master Data Management sync for media fields, source watermarks, contact coupling, blank date fields, deletion-conflict errors, setup pages, and permissions. The remaining high risk is media correctness: preserving the batch cache without replacing stale per-record media state can copy or clear the wrong picture.

Necessity: The change is needed because the fixed scenarios affect sync correctness and setup operability. The scope is appropriate, but the stale per-record media state must be fixed before extending the cache lifetime is safe.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11626 round=2 by=alexei-dobriansky at=2026-09-21T22:22:08.4337067Z lastSha=bb1d8c24af817e668eb18036279e2faa4db548d9 reviewKey=aa9f0eafc645a14e481bf617d5d7d1ac738b3320ce49c0dd619332b601782dcd suggestions=S1@c17d1766:notaddressed parentRound=1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: Apps (W1) Add-on apps for W1 Team: Integrations GitHub request for Integrations area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants