Skip to content
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.

Sign up for GitHub

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

Jump to bottom

No. Series: Ability to extend filters on finding No. Series Lines when getting new numbers #2361

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

No. Series: Ability to extend filters on finding No. Series Lines when getting new numbers #2361

wants to merge 12 commits into from
+30 −6

Conversation

PeterDurrer
Copy link

@PeterDurrer PeterDurrer commented Nov 14, 2024 •
edited by github-actions bot
Loading

Summary

Add Event for No. Series

Work Item(s)

Fixes #1362

Fixes AB#558102

@PeterDurrer PeterDurrer requested a review from a team as a code owner November 14, 2024 08:07
@github-actions github-actions bot added AL: Business Foundation From Fork Pull request is coming from a fork labels Nov 14, 2024
@PeterDurrer PeterDurrer changed the title https://github.com/microsoft/BCApps/issues/1362#issue-2360774022 No. Series: Ability to extend filters on finding No. Series Lines when getting new numbers Nov 14, 2024
@PeterDurrer PeterDurrer reopened this Nov 14, 2024
@PeterDurrer
Copy link
Author

PeterDurrer commented Nov 14, 2024 •
edited
Loading

@PeterDurrer please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Logico Solutions AG"

AndreasMoth
AndreasMoth previously approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@AndreasMoth AndreasMoth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just need the other event to call the new GetNoSeriesLineFilters procedure as well.

grobyns
grobyns requested changes Nov 14, 2024
View reviewed changes
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesSetupImpl.Codeunit.al Outdated Show resolved Hide resolved
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesSetupImpl.Codeunit.al Outdated Show resolved Hide resolved
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesSetupImpl.Codeunit.al Outdated Show resolved Hide resolved
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesSetupImpl.Codeunit.al Outdated Show resolved Hide resolved
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesSetupImpl.Codeunit.al Outdated Show resolved Hide resolved
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesSetupImpl.Codeunit.al Outdated Show resolved Hide resolved
src/Business Foundation/App/NoSeries/src/Single/NoSeries.Codeunit.al Outdated Show resolved Hide resolved
src/Business Foundation/App/NoSeries/src/Single/NoSeries.Codeunit.al Show resolved Hide resolved
src/Business Foundation/App/NoSeries/src/Single/NoSeries.Codeunit.al Outdated Show resolved Hide resolved
src/Business Foundation/App/NoSeries/src/Single/NoSeriesImpl.Codeunit.al Outdated Show resolved Hide resolved
@JesperSchulz JesperSchulz added the Integration GitHub request for Integration area label Nov 14, 2024
@PeterDurrer
Copy link
Author

@grobyns and @AndreasMoth There was some feedback and I have now tried to implement it accordingly. Please check the code again, thank you very much for this intensive review.
@dominicstarkl can you check whether these adjustments provide you with the required code sections that you wanted in issue #1362

@PeterDurrer

This comment has been minimized.

Sign in to view
@grobyns
Copy link
Contributor

grobyns commented Nov 19, 2024

@grobyns Can you please resolve your comments that are know fine for you. Then i can see what i must do to finish the work. Thank you.

done.

We've been discussing the need to add a test or two to verify the error is thrown when the filter on series code changes to ensure future code modifications don't remove that logic.

AndreasMoth
AndreasMoth previously approved these changes Nov 21, 2024
View reviewed changes
grobyns
grobyns previously approved these changes Nov 21, 2024
View reviewed changes
@PeterDurrer
Copy link
Author

@PeterDurrer please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Logico Solutions AG"

@JesperSchulz JesperSchulz added the Linked Issue is linked to a Azure Boards work item label Nov 21, 2024
@github-actions github-actions bot added this to the Version 26.0 milestone Nov 21, 2024
@JesperSchulz JesperSchulz enabled auto-merge (squash) November 21, 2024 10:10
AndreasMoth
AndreasMoth reviewed Nov 21, 2024
View reviewed changes
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesSetupImpl.Codeunit.al Outdated
@@ -15,6 +15,7 @@ codeunit 305 "No. Series - Setup Impl."
NumberFormatErr: Label 'The number format in %1 must be the same as the number format in %2.', Comment = '%1=No. Series Code,%2=No. Series Code';
UnIncrementableStringErr: Label 'The value in the %1 field must have a number so that we can assign the next number in the series.', Comment = '%1 = New Field Name';
NumberLengthErr: Label 'The number %1 cannot be extended to more than 20 characters.', Comment = '%1=No.';
CodeFieldChangedErr: Label 'The filter on %1 was altered by an event subscriber. This is a programming error. Please contact your partner to resolve the issue.\Original %1: %2\Modified Filter: %3';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the error below shows, whenever you add a label with placeholder, you must add a comment specifying what those placeholders are. These are necessary for translation teams to translate the string better. This is required for all strings with placeholders because some string can be very difficult for a translator to understand when they almost fully consist of these placeholders..

AndreasMoth
AndreasMoth reviewed Nov 21, 2024
View reviewed changes
src/Business Foundation/App/NoSeries/src/Setup/NoSeriesSetupImpl.Codeunit.al
procedure SetNoSeriesLineFilters(var NoSeriesLine: Record "No. Series Line"; NoSeriesCode: Code[20]; StartingDate: Date)
var
NoSeries: Codeunit "No. Series";
NoSeriesLine2: Record "No. Series Line";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See errors below, basically the sorting of variables must be fixed, Record comes before codeunit.

auto-merge was automatically disabled November 21, 2024 14:02

Head branch was pushed to by a user without write access

@PeterDurrer PeterDurrer dismissed stale reviews from grobyns and AndreasMoth via 4f1174f November 21, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NKarolak NKarolak NKarolak approved these changes

@AndreasMoth AndreasMoth Awaiting requested review from AndreasMoth

@grobyns grobyns Awaiting requested review from grobyns

At least 2 approving reviews are required to merge this pull request.

Assignees

@AndreasMoth AndreasMoth

@grobyns grobyns

Labels
AL: Business Foundation From Fork Pull request is coming from a fork Integration GitHub request for Integration area Linked Issue is linked to a Azure Boards work item
Projects
None yet
Milestone
Version 26.0
Development

Successfully merging this pull request may close these issues.

No. Series: Ability to extend filters on finding No. Series Lines when getting new numbers
5 participants
@PeterDurrer @grobyns @AndreasMoth @NKarolak @JesperSchulz