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.

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

Daily digest reporting prefill failures for forms that work with more then 1 auth "flow" #4825

Open
Tracked by #4920
LaurensBurger opened this issue Nov 15, 2024 · 4 comments · Fixed by #4959
Open
Tracked by #4920
Assignees
Labels
Milestone

Comments

@LaurensBurger
Copy link
Collaborator

LaurensBurger commented Nov 15, 2024

Product versie / Product version

2.7.8 - 2.8.1

Customer reference

SED 102

Omschrijf het probleem / Describe the bug

Most environments use a common NAW-stap that allows for multiple authentication: DigiD + eHerkenning and anonymous.

These steps usually consist of 2 or 3 field sets and depending on which form of authentication is used, the right field set will be shown with pre-filled values.

In a situation where a users logs into a form with DigiD a request is made to fetch user data from the BRP, and not to the KvK to fetch the business data.

However in the Logs we can see something is going on, but wrongly log a request has been made and the response was empty. These log items also get reported in the daily digest as failures.

Example with eHerkenning login:
image

Example with DigiD login:
image

Both submissions show 1 failed and 1 successful pre-fill attempt. But in both causes the failed attempt seems to be "false" (we don't see any outgoing requests, no sentry errors)

As these steps/forms are used on almost all environments, the extra "failures" on the daily digest cause confusion and hinders the visibility of actual issues.

@LaurensBurger LaurensBurger added bug Something isn't working triage Issue needs to be validated. Remove this label if the issue considered valid. labels Nov 15, 2024
@sergei-maertens
Copy link
Member

we don't see any outgoing requests, no sentry errors

You probably won't ever see sentry errors for this because any errors are suppressed because of a "continue at all costs" philosophy in prefill 😬

@joeribekker joeribekker added topic: email digest topic: tech debt and removed triage Issue needs to be validated. Remove this label if the issue considered valid. labels Nov 18, 2024
@joeribekker
Copy link
Contributor

Refinement: This might be caused by returning an empty list because there is no kvk/bsn which is inadvertently marked as "problem" log entry. There is also overlap between this feature and configuration problem detection. This is some technical debt that needs to be addressed.

Let's first investigate if we can fix it quickly. Please report back if this is not the case.

@vaszig
Copy link
Contributor

vaszig commented Dec 19, 2024

There is a connected PR for this, needs verification from @sergei-maertens that we want such a fix. After some investigation I think I will need more time to handle this properly. The fix has to do only with the real prefill_retrieve_empty log event and when this is triggered.

@vaszig
Copy link
Contributor

vaszig commented Dec 19, 2024

There is a connected PR for this, needs verification from @sergei-maertens that we want such a fix. After some investigation I think I will need more time to handle this properly. The fix has to do only with the real prefill_retrieve_empty log event and when this is triggered.

So, after a discussion with Sergei as well, we concluded that we do not want to add a quick fix like this. This will not fix the real problem which goes a bit deeper.

What we have now is that we prefill the values for each plugin but without checking the used authentication flow (when login is required). So, it's logical to get an extra log entry (for each unused authentication plugin) prefill_retrieve_empty, since the values we get back is an empty list.

This should be discussed again, maybe during refinement.

@vaszig vaszig added blocked needs-backport Fix must be backported to stable release branch and removed blocked labels Dec 19, 2024
vaszig added a commit that referenced this issue Dec 20, 2024
This is a fix which is meant for backporting. The 'proper' fix will be
implemented in a different PR, out of the scope of the v3.0.
vaszig added a commit that referenced this issue Dec 20, 2024
This is a fix which is meant for backporting. The 'proper' fix will be
implemented in a different PR, out of the scope of the v3.0.
vaszig added a commit that referenced this issue Dec 23, 2024
…s-prefill-failures-when-multiple-auth-flows

[#4825] Log prefill retrieve empty only for the used authentication flow
@github-project-automation github-project-automation bot moved this from In Progress to Done in Development Dec 23, 2024
@vaszig vaszig moved this from Done to In Progress in Development Dec 23, 2024
vaszig added a commit that referenced this issue Dec 23, 2024
This is a fix which is meant for backporting. The 'proper' fix will be
implemented in a different PR, out of the scope of the v3.0.

Backport-of: #4959
vaszig added a commit that referenced this issue Dec 23, 2024
This is a fix which is meant for backporting. The 'proper' fix will be
implemented in a different PR, out of the scope of the v3.0.

Backport-of: #4959
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

4 participants