-
Notifications
You must be signed in to change notification settings - Fork 31
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
Feat/265 url param instantiation #2951
base: main
Are you sure you want to change the base?
Conversation
/publish |
PR release:
|
/publish |
PR release:
|
|
}: DataModelDeps & DataModelProps) { | ||
if (prefillFromQueryParams && isStateless && !isAnonymous && dataType) { |
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.
I don't see why we should limit this not to work for anonymous stateless? 🤔
includeRowIds: boolean, | ||
prefillFromQueryParams: string, | ||
) => | ||
`${appPath}/v1/data?dataType=${dataType}&includeRowId=${includeRowIds.toString()}&prefill=${prefillFromQueryParams}`; |
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.
So you're passing prefill to the backend so that backend fills that into the model when first reading it? That's not quite what I expected here - I thought we talked about sending the first request as a POST with the prefill data in it, not sending a GET with prefill data for the backend to figure out. This works, I guess, but it adds complexity when we could have done this with existing APIs already (but then via POST).
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.
Uuuhm well wouldnt that require a new implementation instead of adding to what already happens?
Wouldnt that add even more complexity?
Either way I think it makes sense to remove the GET /data endpoint altogether in this task: #2065 as the GET and POST do more or less the same thing, so could merge this and fix it as part of that I guess.
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.
Or did you mean that we just do a setValue or something? We also have to let TE run validation code of the queryparam values so that would have to hook into it somehow also, see line 132 here: https://github.com/Altinn/app-lib-dotnet/pull/1068/files#diff-57722a0d58a9aa420ee547af44fab41e404a5a7fb409e39c6e749569410644fd
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.
Aha! Yep, I think it makes sense to never call the GET endpoint at all, and just start off by POSTing an empty object (or one with prefill) in stateless. Might want to double-check that with Ivar as well, but I think that would work.
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.
So to summarise:
- Stateless currently create the initial data model with a call to GET, and does subsequent updates with POST
- We think that the initial data model can be an empty object
{}
and then we can use POST all the time.
One disadvantage is that this means that backend can only have one hook, instead of initialize
and update
. It can run initialisation on update, if it sets {"isInitalized": true}
, so I think we can accept that for future extensibility.
Seems like for some reason we do prefill
on both POST
and GET
,
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.
After a huddle we have decided it makes sense to switch to using the POST endpoint and deprecate the GET endpoint.
Description
Allows users to configure query params that will prefill the datamodel.
Depends on this backend PR: Altinn/app-lib-dotnet#1068
Query params will prefill datamodel according to this flow diagram:
Related Issue(s)
Verification/QA
kind/*
label to this PR for proper release notes grouping