-
Notifications
You must be signed in to change notification settings - Fork 687
SL Cloud Migration Update November 2025 #29477
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
base: main
Are you sure you want to change the base?
Conversation
…ForFieldUpdate to a public procedure
…ns, and remove obsolete codeunit
|
Could not find linked issues in the pull request description. Please make sure the pull request description contains a line that contains 'Fixes #' followed by the issue number being fixed. Use that pattern for every issue you want to link. |
|
@nikolakukrika This PR is now ready for code review. Thank you. |
…te for assigning dimensions on Beginning Balance journal entries.
…nto SLMigrationUpdate2025-11
…r, and SL Batch tables to support tests.
| SLPopulateVendor1099Data: Codeunit "SL Populate Vendor 1099 Data"; | ||
| begin | ||
| if (SLCompanyAdditionalSettings.GetMigrateCurrent1099YearEnabled()) or (SLCompanyAdditionalSettings.GetMigrateNext1099YearEnabled()) then begin | ||
| SetupIRSFormsFeatureIfNeeded(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GP Migration has this method EnsureSupportedReportingYear.
Do we need to do additional checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need additional checks to ensure its a supported reporting year. In SL, there are only two 1099 years; the Current 1099 Year and Next 1099 Year.
| SLPopulateVendor1099Data.Run(); | ||
| UnbindSubscription(SLPopulateVendor1099Data); | ||
| end; | ||
| end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GP Migration has SetPreferredVendorBankAccountsUseForElectronicPayments();
Do we need this method too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not believe so. SL does not have a preferred Vendor Bank Accounts for use for electronic payments
| SLVendor1099MappingHelpers.InsertSupportedTaxYear(TaxYear); | ||
|
|
||
| // MISC | ||
| SLVendor1099MappingHelpers.InsertMapping(TaxYear, '1', '1M', 'MISC', 'MISC-01'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have obsoleted the old IRS1099 functionality. Are you sure that this is the correct implementation?
The GP code looks different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is the correct implementation. The Create1099Mappings procedure is similar to the Install2022Mappings procedure in GP. SL only supports the 1099 MISC and NEC-01 boxes. It does not support the 1099-DIV and INT amounts.
| IRS1099Code: Code[10]; | ||
| TaxAmount: Decimal; | ||
| begin | ||
| if SLAPBalances.Get(VendorNo, CompanyName) then begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This level of nesting is too much. The better is
if SLAPBalances.Get(VendorNo, CompanyName) then
exit;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored the code to be more readable
| VendorYear1099AmountDictionary.Add(IRS1099Code, SLAPBalances.CYBox13); | ||
| if SLAPBalances.CYBox12 > 0 then | ||
| IRS1099Code := SLVendor1099MappingHelpers.GetIRS1099BoxCode(TaxYear, '25') | ||
| else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these can be refactored to:
if SLAPBalances.CYBox12 > 0 then begin
IRS1099Code := SLVendor1099MappingHelpers.GetIRS1099BoxCode(TaxYear, '25')
if IRS1099Code <> '' then
if VendorYear1099AmountDictionary.Get(IRS1099Code, TaxAmount) then
VendorYear1099AmountDictionary.Set(IRS1099Code, TaxAmount + SLAPBalances.CYBox12)
else
VendorYear1099AmountDictionary.Add(IRS1099Code, SLAPBalances.CYBox12);
end;
It is more readable and less dependencies on clearing the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored the code to be more readable
| GenJournalBatch.SetRange(Name, GeneralJournalBatchCode); | ||
| GenJournalBatch.SetRange("No. Series", NoSeriesCode); | ||
| GenJournalBatch.SetRange("Posting No. Series", PostingNoSeriesCode); | ||
| if not GenJournalBatch.FindFirst() then begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if GenJournalBatch.IsEmpty() then begin
We are not using the value anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code to use "if GenJournalBatch.IsEmpty() then begin" as suggested
| begin | ||
| // [Given] SL Data | ||
| Initialize(); | ||
| SLTestHelperFunctions.ClearBCVendorTableData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three lines should be a part of the initialize method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines have been added to the initialize method
| TempVendor.FindSet(); | ||
| repeat | ||
| Assert.IsTrue(Vendor.Get(TempVendor."No."), 'Vendor record not found for No. ' + TempVendor."No."); | ||
| Assert.AreEqual(TempVendor."Federal ID No.", Vendor."Federal ID No.", 'Federal ID No. does not match for Vendor No. ' + TempVendor."No."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration code is moving more, balances, payment entries. Should we check these as well?
We should get asserts for all things we are migrating
| CreateOpeningBalanceTrx(Sender, RecordIdToMigrate); | ||
| end; | ||
|
|
||
| internal procedure CreateOpeningBalanceTrx(var Sender: Codeunit "GL Acc. Data Migration Facade"; RecordIdToMigrate: RecordId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add tests for these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests for GL Account beginning balances, along with tests for open AP and open AR balances have been pushed up to this PR.
| IRS1099VendorFormBoxSetup: Record "IRS 1099 Vendor Form Box Setup"; | ||
| begin | ||
| SLCompanyAdditionalSettings.GetSingleInstance(); | ||
| if IRS1099VendorFormBoxSetup.Get(TaxYear, Vendor."No.") then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exit(false) is missing at the bottom.
It is better to refactor to exit(IRS1099VendorFormBoxSetup.Get(TaxYear, Vendor."No."));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored code as suggested
| if GLAccount.Get(VendorPostingGroup."Payables Account") then | ||
| exit(GLAccount."No."); | ||
|
|
||
| exit(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be blank or DefaultPayablesAccountCode?
If it should be blank can we get a comment explaining why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced exit(''); with exit(DefaultPayablesAccountCode);
|
|
||
| CreateMappingsIfNeeded(); | ||
|
|
||
| Evaluate(Day31, Day31Txt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is simpler to use PostingDate := DMY2Date(31, 12, CurrentYear);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be simpler but I wasn't sure if the 31 and 12 would be considered "hard-coded" values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed code to "PostingDate := DMY2Date(31, 12, CurrentYear);" as suggested
| CurrentYearJournalBatchName := VendorTaxBatchNameTxt + Format(SLCurr1099Yr); | ||
| GenJournalLine.SetRange("Journal Batch Name", CurrentYearJournalBatchName); | ||
| GenJournalLine.SetFilter("Account No.", '<>%1', ''); | ||
| if (not GenJournalLine.IsEmpty()) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the correct logic? If we do not have a current year but we have a next year, we will exit and not clean the next year.
Do all the exits in this method make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored the CleanUp procedure to accept an Year value and updated code to call the CleanUp procedure for both the Current and the Next 1099 Years, Also, removed unnecessary exits
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple blank lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to explain in a little more detail? Is there a place where blank lines should be added or removed?
| ErrorMessageInStream.ReadText(ErrorMessageLine); | ||
| ErrorMessageBuilder.AppendLine(ErrorMessageLine); | ||
| end; | ||
| exit(ErrorMessageBuilder.ToText().Trim()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - missing terminating ; on the line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated code to "exit(ErrorMessageBuilder.ToText().Trim());"
| SLCompanyAdditionalSettings: Record "SL Company Additional Settings"; | ||
| IRS1099VendorFormBoxSetup: Record "IRS 1099 Vendor Form Box Setup"; | ||
| begin | ||
| SLCompanyAdditionalSettings.GetSingleInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SLCompanyAdditionalSettings are not used after this line. Is it a bug or should the reference be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed SLCompanyAdditionalSettings references
…nto SLMigrationUpdate2025-11
…r message handling in migration log.
Summary
SL Cloud Migration Update for November 2025
This PR replaces closed PRs 29378 and 29394.
Work Item(s)
Fixes #
AB#575741
AB#609658
AB#612598