-
Notifications
You must be signed in to change notification settings - Fork 169
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
[DRAFT] #845 Added No. Series Where-Used #846
Conversation
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.
Just some super-minor findings along your way :-)
I did not check the business logic itself.
TempNoSeriesWhereUsed: Record "No. Series Where-Used" temporary; | ||
NextEntryNo: Integer; | ||
|
||
procedure ShowSetupForm(NoSeriesWhereUsed: Record "No. Series Where-Used") |
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.
"Form"? :-)
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesCalcWhereUsed.Codeunit.al
Outdated
Show resolved
Hide resolved
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesCalcWhereUsed.Codeunit.al
Outdated
Show resolved
Hide resolved
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesCalcWhereUsed.Codeunit.al
Outdated
Show resolved
Hide resolved
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesCalcWhereUsed.Codeunit.al
Outdated
Show resolved
Hide resolved
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesCalcWhereUsed.Codeunit.al
Outdated
Show resolved
Hide resolved
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesCalcWhereUsed.Codeunit.al
Outdated
Show resolved
Hide resolved
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesCalcWhereUsed.Codeunit.al
Outdated
Show resolved
Hide resolved
…reUsed.Codeunit.al Co-authored-by: Natalie Karolak <[email protected]>
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.
Apply suggestions from code review
Co-authored-by: Natalie Karolak <[email protected]>
we've been discussing your PR a little and we're now having some doubts about its validity. While it makes a ton of sense to be able to see where a No. Series is used, we're not sure that this feature should be build in the way it's being built right now. In AI, "where used" is a rather prominent capability, which must be generally available for all entity relationships. You will probably see some advancements in this area in the near future, and I hence think it makes sense to wait to add this capability, until we know what the future holds here. If you agree, I'd suggest to close this DRAFT for now and pick it up again, once we know more. What do you say? |
I completely 100% agree. Once I moved the code into the business foundation layer, everything felt out of place, and even before that, there's a need for more general functionality that would benefit all the other modules. So a WhereUsed module would make perfect sense. The same goes for the approval workflows imho, but I'll get to that in the otter PR. |
Good to hear that you've made the same observations 😊 Let's see what the future holds! |
Summary
This is a Draft PR. There's still a lot to do.
Work Item(s)
Fixes #845