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 #1362

Open
1 task done
dominicstarkl opened this issue Jun 18, 2024 · 5 comments · May be fixed by #2361
Open
1 task done

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

dominicstarkl opened this issue Jun 18, 2024 · 5 comments · May be fixed by #2361
Labels
Approved The issue is approved Bug Something isn't working Integration GitHub request for Integration area

Comments

@dominicstarkl
Copy link

Describe the issue

In the current implementation it is not possible to set additional filters when matching No. Series Lines are filtered on getting new numbers, see procedure GetNoSeriesLine(), codeunit 304 "No. Series - Impl.".

The only way to extend the filtering right now is by subscribing to IntegrationEvent OnAfterSetNoSeriesCurrentLineFilters(), called in procedure SetNoSeriesCurrentLineFilters(), codeunit 305 "No. Series - Setup Impl.".
Obviously the No. Series Lines are filtered there for UI purposes when setting up new No. Series and not for getting new numbers as in GetNoSeriesLine().

Before the movement of No. Series module to Business Foundation filters applied to No. Series Lines could be extended by subscribing to OnNoSeriesLineFilterOnBeforeFindLast() called in SetNoSeriesLineFilter() of codeunit 396 NoSeriesManagement, which is now obsoleted.

The filtering in SetNoSeriesLineFilter() served both for UI and getting new numbers purposes, whereas in the new solution the filtering is done in separate places (codeunits 304 and 305).
Therefore OnAfterSetNoSeriesCurrentLineFilters() does not completely replace former OnNoSeriesLineFilterOnBeforeFindLast() - there is no ability to extend filters on getting new numbers as it was in the old solution.

Expected behavior

There should be the possibility of extending filters applied to No. Series Lines in GetNoSeriesLine(), codeunit 304 "No. Series - Impl.", as a partial replacement of former OnNoSeriesLineFilterOnBeforeFindLast() in order to unbreak current subscribers.

The new IntegrationEvents could be raised in the same manner as the obsoleted OnNoSeriesLineFilterOnBeforeFindLast() is currently raised. Suggested signature: OnGetNoSeriesLineOnBeforeFindLast(var NoSeriesLine: Record "No. Series Line")

Steps to reproduce

Use case: No. Series Lines are filtered by department of current user.

Example:

[EventSubscriber(ObjectType::Codeunit, Codeunit::NoSeriesManagement, OnNoSeriesLineFilterOnBeforeFindLast, '', false, false)]
local procedure NoSeriesManagement_OnNoSeriesLineFilterOnBeforeFindLast(var NoSeriesLine: Record "No. Series Line")
begin
	NoSeriesLine.SetFilter("Department", '%1|%2', '', UserSetup.GetDepartmentCode());
end;

Additional context

No response

I will provide a fix for a bug

  • I will provide a fix for a bug
@dominicstarkl dominicstarkl added the Bug Something isn't working label Jun 18, 2024
@grobyns
Copy link
Contributor

grobyns commented Jun 25, 2024

Hi @dominicstarkl

I'd like get your thoughts on some things before going ahead and adding the event. There's a few points of concern:

  • since the event would pass the No. Series Line as var in order for additional/other filters to be added, there's a possibility of completely changing the series you're getting numbers from. Unfortunately I don't think we have a good way of capturing only added filters in a good way.
  • Have you checked whether a single raising of the event in the new code would suffice for your scenario? We do few rounds of setting and unsetting certain filters and those might interfere with your scenario. Ideally you'd just set your filter at the start and everything else should just work.

Another option which I tend to favour is to add an interface for the GetNoSeriesLine method and let you define your own implementation. This would require a little more rework on your end but in exchange you'd gain full control. You could then add the implementation as a setting on the No. Series Line so it would be very explicit.

I look forward to your comments,
br/Gert

@dominicstarkl
Copy link
Author

dominicstarkl commented Jun 25, 2024 •
edited
Loading

@grobyns
Hi Gert

Many thanks for your reply and time for our issues.

  1. I agree, partners/PTEs could extend the behaviour of finding No. Series Lines and may completely break the originators/microsoft intention. IMHO this may always be the case when subscring to an event, it's the nature of the event/subscriber model. BTW this is currently also the case when subscribing to the existing event when filters are composed on setting up No. Series Lines.

  2. I have analyzed the current implementation and found out that adding the event publishers on the same places as in the deprecated solution would give subscribers the same control as they had before.

  3. Interfacing is always a good option in many places. But in this case we would appreciate the event/subscriber model to keep it simple as it was before.

What are the next steps?
Can I make a pull request with new events or would you do that for us?

Is it the correct way to ask for events/extensibility of BCApps/Business Foundation App by creating a bug issue on this GitHub repo in the future?
I saw that there is currently just one event in the whole Business Foundation App. Partners may probably ask for further events/extensibility in the future.
Or can we place such requests in the ALAppExtensions repo just as we do now for the Base Application?

Kindly regards
Dominic

@grobyns
Copy link
Contributor

grobyns commented Jun 26, 2024

Hi @dominicstarkl,

For anything on BCApps the approach is to create an issue and if/when approved create the PR, that includes event requests. We will very carefully evaluate every event request to make sure it's the right event, with the right parameters and name and in the right place, and where possible, we'll promote alternatives.

For this particular issue, I'll approve and you can create a PR. I'd suggest OnGetNoSeriesLineFilters (or similar) as event name and the documentation of the event publisher should make it clear the purpose is only to add additional filters to the line. I'd also add a testfield or similar check on the Series Code field to ensure we get the same series before and after the event.

br/Gert

@grobyns grobyns added the Approved The issue is approved label Jun 26, 2024
This was referenced Jun 26, 2024
Closed
Closed
@grobyns
Copy link
Contributor

grobyns commented Sep 12, 2024

@dominicstarkl I just noticed this issue is still open. Is anything pending or can it be closed?

@dominicstarkl
Copy link
Author

dominicstarkl commented Sep 12, 2024 •
edited
Loading

@grobyns
Hi Gert

Thanks for asking. I'm still on holidays.
I will provide a PR asap, hopefully until end of this week.

Cheers
Dominic

@JesperSchulz JesperSchulz added the Integration GitHub request for Integration area label Sep 16, 2024
@PeterDurrer PeterDurrer linked a pull request Nov 14, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Approved The issue is approved Bug Something isn't working Integration GitHub request for Integration area
Projects
None yet
Milestone
No milestone
3 participants
@JesperSchulz @grobyns @dominicstarkl