-
Notifications
You must be signed in to change notification settings - Fork 319
Added ABC Analysis Setup to Power BI Reports BC app #5869
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?
Added ABC Analysis Setup to Power BI Reports BC app #5869
Conversation
|
@mynjj Can you review this PR for Power BI Reports BC app please? |
|
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. |
src/Apps/W1/PowerBIReports/App/Inventory/Pages/ABCAnalysisSetup.Page.al
Outdated
Show resolved
Hide resolved
| UpgradeTag.SetUpgradeTag(CloseIncomeSourceCodeUpgradeTag()); | ||
| end; | ||
|
|
||
| local procedure InitializeABCAnalysisSetupUpgrade() |
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.
From what I see that the initialization does, we could do this on the fly instead of at upgrade. Let's do that, let's say when opening the main setup page, and if the setup is needed and it's not there, return default 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.
+1 for this approach.
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.
@tuan-nguyen-fenwick , what do you think about this suggestion? to initialize on the fly instead of needing to add upgrade code
src/Apps/W1/PowerBIReports/App/Inventory/Embedded/Inventory/ABCAnalysis.Page.al
Outdated
Show resolved
Hide resolved
src/Apps/W1/PowerBIReports/App/Inventory/Pages/ABCAnalysisSetup.Page.al
Outdated
Show resolved
Hide resolved
| var | ||
| SetupHelper: Codeunit "Power BI Report Setup"; | ||
| ReportId: Guid; | ||
| ReportPageLbl: Label 'a476d6afc8d5d544193b', Locked = true; |
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 report id doesn't seem to be in yet, should we include it in this PR? I'm also fine if you want to keep it in a separate PR
src/Apps/W1/PowerBIReports/App/Inventory/Tables/ABCAnalysisSetup.Table.al
Outdated
Show resolved
Hide resolved
src/Apps/W1/PowerBIReports/App/Inventory/Pages/ABCAnalysisSetup.Page.al
Outdated
Show resolved
Hide resolved
src/Apps/W1/PowerBIReports/App/Inventory/Tables/ABCAnalysisSetup.Table.al
Outdated
Show resolved
Hide resolved
| UpgradeTag.SetUpgradeTag(CloseIncomeSourceCodeUpgradeTag()); | ||
| end; | ||
|
|
||
| local procedure InitializeABCAnalysisSetupUpgrade() |
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.
@tuan-nguyen-fenwick , what do you think about this suggestion? to initialize on the fly instead of needing to add upgrade code
| tabledata "PowerBI Reports Setup" = RIMD, | ||
| tabledata "Working Day" = RIMD, | ||
| tabledata "Account Category" = RM; | ||
| tabledata "Account Category" = RMID, |
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.
why did we add the "ID" to account category for this functionality?
| tabledata "Working Day" = RIMD, | ||
| tabledata "Account Category" = RM; | ||
| tabledata "Account Category" = RMID, | ||
| tabledata "PowerBI ABC Analysis Setup" = RMID; |
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.
very minor 😅 : it's usually RIMD (in that order)
src/Apps/W1/PowerBIReports/App/Inventory/Embedded/Inventory/ABCAnalysis.Page.al
Outdated
Show resolved
Hide resolved
| // ------------------------------------------------------------------------------------------------ | ||
| namespace Microsoft.Inventory.PowerBIReports; | ||
|
|
||
| page 36982 "PowerBI ABC Analysis Setup" |
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.
same here, you were missing to rename the file
| // ------------------------------------------------------------------------------------------------ | ||
| namespace Microsoft.Inventory.PowerBIReports; | ||
|
|
||
| table 36956 "PowerBI ABC Analysis Setup" |
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.
missing to rename the file
|
|
||
| local procedure InitializeABCAnalysisSetupUpgradeTag(): Code[250] | ||
| begin | ||
| exit('MS-TEMP-POWERBI-ABC-ANALYSIS-SETUP-20251216'); //TODO: To be confirmed with Microsoft team. |
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.
Ok, so you need workitem No. It is 606229. The code could be like this:
exit('MS-606229-PowerBIABCAnalysisSetup-20251216')
| UpgradeTag.SetUpgradeTag(CloseIncomeSourceCodeUpgradeTag()); | ||
| end; | ||
|
|
||
| local procedure InitializeABCAnalysisSetupUpgrade() |
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.
+1 for this approach.
PredragMaricic
left a comment
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.
Please fix comments above
@PredragMaricic Thank you, I've updated the upgrade tag. All other comments can be resolved. |
|
@PredragMaricic |
Prerequisite
Requires PR completion: https://github.com/microsoft/BusinessCentralApps/pull/1757
Change summary
New ABC Analysis page for Inventory Power BI Report:
Fixes AB#606229