[FS] Prevent duplicate Field Service customer assets during item synchronization - #11094
tomasevicst wants to merge 39 commits into
Conversation
github-actions
Bot
added
AL: Apps (W1)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
The removed IgnoreServiceItem* tests, and the new ItemSynchronizationDisablesCustomerAssetConversion test, both call the helper procedure directly rather than exercising the actual dispatch path. Critically, the new 'Item-CRM Product' case added to OnBeforeTransferFieldData (which does DestinationRecordRef.SetTable(CRMProduct) / DisableCustomerAssetConversion(...) / DestinationRecordRef.GetTable(CRMProduct)) is never exercised by any test — no test drives an Item-to-CRMProduct transfer through the RecordRef dispatch to confirm the case is reached and the record is written back correctly. Add an integration-style test that triggers the Item-CRM Product transfer path (e.g. via the subscriber's OnBeforeTransferFieldData or a full sync call) and asserts the resulting CRM Product has ConvertToCustomerAsset = false. Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6 |
…ronization logic for service items
…procedure to streamline service item synchronization
…re and update test cases for service item synchronization
github-actions
Bot
added
Team: Other
GitHub Actions
Active
github-actions
Bot
added
Team: SCM
Good Sense Reviewer - Round 1Recommendation: Accept with SuggestionsWhat this PR doesThis change keeps Business Central as the source for Field Service customer assets for synchronized items. It removes the service-item filter that depended on the Product's Convert to Customer Asset flag, and it adds item-product synchronization logic that turns that flag off for coupled items. The main behavior looks correct for the reported duplicate-asset scenario. The service-item mapping depends on the item-product mapping, the new item-product transfer path uses the coupling state to derive the Product value, and the tests cover both the mapping setup and a coupled item/product synchronization. The relevant integration event publishers and call sites were checked, including the field-modified and transfer-field hooks used by this change. Problem-solution fitFit: Strong The reported problem is that both systems can create a customer asset for the same equipment. The change addresses the root cause by always allowing the Business Central service item to synchronize and by disabling native Field Service customer asset conversion on coupled Products. SuggestionsS1 (🟠 Moderate): Guard field-modified handling when disabled Risk assessment and necessityRisk: The main regression surface is item-to-Product synchronization and service-item-to-customer-asset synchronization for coupled Field Service data. The change is not financial posting logic and does not change schema, but it changes integration behavior for coupled Products and can affect customers that rely on native Field Service conversion for those Products. Necessity: The change is needed because the old Product flag could suppress Business Central customer-asset synchronization while still allowing Field Service to create another asset during work order completion. The scope is targeted: it keeps service items synchronizing and moves the conflict prevention to the item-product mapping where the Product flag is maintained.
|
…scriber and add corresponding test case
Good Sense Reviewer - Round 2Recommendation: AcceptWhat this PR doesThe latest change addresses the remaining field-comparison gap from round 1. The field-modified subscriber now uses the same Field Service enabled guard as the transfer subscriber, and the added test verifies that the comparison hook stays unhandled when integration is disabled. The current diff still fits the reported duplicate-customer-asset scenario. The item-product mapping derives Convert to Customer Asset from the item coupling state, service items are no longer skipped by that Product flag, and the comparison and transfer hooks now stay consistent when Field Service integration is off. Status of previous suggestions
New observations (commits since round 1)None - changes only addressed the previous suggestion. Risk assessment and necessityRisk: The regression surface remains item-to-Product synchronization and service-item-to-customer-asset synchronization for coupled Field Service data. The latest change lowers the disabled-integration risk by making field comparison and field transfer follow the same guard. Necessity: The change is still needed to prevent both systems from creating customer assets for the same equipment. The round-2 change is also necessary because it keeps the new mapping inactive when Field Service integration is disabled.
|
…sset conversion checks and update corresponding test case
Good Sense Reviewer - Round 3Recommendation: AcceptWhat this PR doesThe latest change keeps the accepted Field Service customer-asset fix but simplifies how the synchronization hooks read the item managed state. Instead of rebuilding an Item record from the source field, both hooks now use the already-calculated source field value, so the comparison and transfer paths use the same boolean input. The current PR still fits the reported duplicate-customer-asset scenario. The item-product mapping derives Convert to Customer Asset from the item coupling state, service items are no longer skipped by that Product flag, and the latest commit does not change the intended behavior. Status of previous suggestions
New observations (commits since round 2)None - the latest change is a narrow refactor and test cleanup. Risk assessment and necessityRisk: The regression surface remains item-to-Product synchronization and service-item-to-customer-asset synchronization for coupled Field Service data. The latest refactor uses the already-calculated source field value consistently in comparison and transfer hooks, so it does not add a new compatibility or data-integrity risk. Necessity: The change is still needed to prevent both systems from creating customer assets for the same equipment. The round-3 change is also reasonable because it removes unnecessary record reconstruction and keeps the hook logic focused on the mapped field value.
|
Good Sense Reviewer - Round 4Recommendation: Request ChangesWhat this PR doesThe latest change makes the field-modified check read the persisted Product value before deciding whether Convert to Customer Asset changed. That addresses the comparison problem for an existing Product. The full PR still does not fully close the duplicate-customer-asset scenario. The new mapping derives the Product flag from the Item coupling FlowField, but that FlowField is false during the first Item-to-Product insert because the coupling is created after the Product is inserted. Status of previous suggestions
New observations (commits since round 3)S2 (🔴 High): Initial item sync still enables conversion Risk assessment and necessityRisk: The remaining risk is data duplication for Products first created from Business Central Items. The sync engine transfers fields before inserting the Product and before updating the integration coupling, so the initial Product can keep native Field Service customer-asset conversion enabled. Necessity: The change is needed, but it must disable Field Service customer-asset conversion on the first Product created from a Business Central Item as well as on later syncs of already-coupled Products.
|
…ate_Field_Service_Customer_Asset_Created
…tomer asset conversion logic
…tem synchronization and add corresponding test case
Good Sense Reviewer - Round 5Recommendation: Accept with SuggestionsWhat this PR doesThe latest changes replace the Item coupling-derived mapping with a constant false mapping and enforce the same value after field transfer. The first Item-to-Product synchronization is now handled before the Product is inserted or coupled, and a full synchronization test verifies that the new Product has conversion disabled. The current diff addresses the duplicate-asset scenario for both new and existing Products. Existing Products are checked against their persisted value before being changed, while Service Items continue to synchronize. Status of previous suggestions
New observations (commits since round 4)S3 (🟠 Moderate): Preserve the obsolete helper behavior until removal Risk assessment and necessityRisk: The production fix is targeted to Item-to-Product and Service Item synchronization. The remaining risk is limited to downstream test apps that call the public test-library helper and now receive different behavior during its obsoletion window. Necessity: The change is needed to prevent both systems from creating customer assets for the same equipment. The constant mapping and runtime enforcement now cover both the first Product synchronization and later synchronization of existing Products.
|
…ate related test case
Good Sense Reviewer - Round 6Recommendation: Accept with SuggestionsWhat this PR doesThe latest change now disables customer asset conversion immediately before a Product is inserted or modified. For an existing Product, synchronization is forced only when the persisted value is still enabled; the first-sync test now explicitly allows creation of an uncoupled Product. This keeps the value enforcement in the record-writing path and covers both new and existing Products without changing Service Item synchronization. Status of previous suggestions
New observations (commits since round 5)None - the latest commit refines the Product insert and modify paths and updates the first-synchronization test. Risk assessment and necessityRisk: The production behavior is now enforced directly before Product writes, and the existing regression tests cover new and coupled Products. The remaining risk is limited to downstream test apps that call the obsolete public test helper and now receive different behavior during its obsoletion window. Necessity: The change is needed to prevent Field Service and Business Central from creating duplicate customer assets for the same equipment. The current implementation covers initial synchronization and later updates while Service Items continue to synchronize.
|
Use the standard mock CRM connection for item-product synchronization tests and assert the synchronization job succeeds before validating coupling and customer asset conversion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good Sense Reviewer - Round 7Recommendation: Accept with SuggestionsWhat this PR doesThe latest change uses the standard mock connection for the item-to-Product synchronization tests and now verifies that each synchronization job completes successfully before checking the Product result. The full change still addresses the duplicate-customer-asset scenario. The integration sync events expose the destination record before insert and modify, so conversion is disabled in the record-writing path for both new and existing Products. The updated tests now exercise those paths and confirm one inserted or modified record with no synchronization failure. Status of previous suggestions
New observations (commits since round 6)None - the latest commit repairs the test connection setup and adds explicit synchronization-job assertions. Risk assessment and necessityRisk: The production behavior remains targeted to Item-to-Product and Service Item synchronization. The latest test-only change reduces uncertainty by proving the insert and modify synchronization jobs succeed; the remaining compatibility risk is limited to downstream test apps that call the obsolete public helper during its obsoletion window. Necessity: The production change is needed to prevent Field Service and Business Central from creating duplicate customer assets for the same equipment. The latest test setup change is also necessary because it makes the regression tests execute the real synchronization path and fail clearly if that path does not complete.
|
…and update integration tests
Good Sense Reviewer - Round 8Recommendation: Accept with SuggestionsWhat this PR doesThe latest commit replaces the direct The full fix remains correct. New Products have customer asset conversion disabled before insert. For existing Products, the persisted value triggers a modify when needed, and conversion is disabled before the record is written. The latest refactor preserves this flow and matches the established BaseApp pattern. Status of previous suggestions
New observations (commits since round 7)None - the latest refactor preserves the Product insert and modify behavior and improves synchronization-test diagnostics. Risk assessment and necessityRisk: The production behavior remains targeted to Item-to-Product and Service Item synchronization. The typed RecordRef update follows the established integration pattern, and the regression tests cover new and existing Products. The remaining compatibility risk is limited to downstream test apps that call the obsolete public helper during its obsoletion window. Necessity: The change is needed to prevent Field Service and Business Central from creating duplicate customer assets for the same equipment. The latest refactor is also useful because it keeps record mutation explicit and makes synchronization-test failures easier to diagnose.
|
Create Item Unit of Measure couplings required by the Item-Product synchronization dependency before exercising initial and existing product synchronization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good Sense Reviewer - Round 9Recommendation: Accept with SuggestionsWhat this PR doesThe latest commit couples each Item Unit of Measure to a CRM UoM before running the Item-to-Product synchronization tests. This adds a required synchronization dependency, but the jobs still stop before reaching the Product assertions because the CRM base currency is not initialized. The production fix remains correct. The constant mapping disables native customer-asset conversion, the insert hook handles new Products, and the modify path checks the persisted value and disables conversion before writing existing Products. The BaseApp publisher timing supports both hooks. Status of previous suggestions
New observations (commits since round 8)S4 (🟠 Moderate): Initialize CRM base currency in sync tests Risk assessment and necessityRisk: The production behavior remains targeted to Item-to-Product and Service Item synchronization, and the BaseApp event sequence supports the insert and modify enforcement. Validation remains incomplete because both behavioral synchronization tests fail during setup. The obsolete public helper also still changes behavior for downstream test apps during its obsoletion window. Necessity: The production change is needed to prevent Business Central and Field Service from creating duplicate customer assets for the same equipment. The latest UoM setup is necessary, but the tests also need a configured CRM base currency before they can validate the fixed paths.
|
Create the mock CRM organization and copy its base currency to the CRM connection before running Item-Product synchronization scenarios. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This branch was successfully deployed
3 participants
What & why
Business Central and Field Service could both create a Customer Asset for the same item, resulting in duplicates when a Work Order was completed.
This change keeps Business Central as the source for Customer Asset creation by:
Linked work
Fixes AB#649195
How I validated this
What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)
Risk & compatibility
Risk is low and limited to coupled Field Service Products. Their Convert to Customer Asset value is overwritten to No during Item synchronization, preventing native Field Service asset creation.
Uncoupled Field Service Products are unaffected. No schema or public API changes are introduced. Existing configurations are covered by runtime enforcement, while reset or newly created mappings receive the constant outbound mapping.