Skip to content

feature: include what field/component that triggered save #506

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

Merged
merged 2 commits into from
Sep 30, 2022

Conversation

lorang92
Copy link
Contributor

@lorang92 lorang92 commented Sep 29, 2022

Description

Adds two header values field and componentId when calling put on a data element (which in turns calls processWrite).
This enables app developers with some c# knowhow to get the header values from the HTTP-context when running data processing.

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

@lorang92 lorang92 changed the title feature: include what field + component that triggered save feature: include what field/component that triggered save Sep 29, 2022
@lorang92 lorang92 self-assigned this Sep 30, 2022
Copy link
Contributor

@olemartinorg olemartinorg left a comment

Choose a reason for hiding this comment

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

While I'm not opposed to this quick-fix (I know we've discussed this for a long time, and there is certainly a need for it), I still think the question is a bit misunderstood. App developers want to know "which field changed here", but with repeating groups it can be an entire row at once. There is also no guarantee there won't be changes in app-frontend-react causing multiple values to change at the same time. Also, what if you have a spotty internet connection and some requests to save will fail, some will be delayed - that might make the backend miss the event it was looking for. Relying on getting the specific component ID that made the change, and trusting you'll always get that event, I think it's a recipe for subtle bugs in the future. The "proper" way (I think) should be to use a diffing tool in the backend to generate a list of what changed from before, and we already have the tools for that there.

@lorang92
Copy link
Contributor Author

lorang92 commented Sep 30, 2022

While I'm not opposed to this quick-fix (I know we've discussed this for a long time, and there is certainly a need for it), I still think the question is a bit misunderstood. App developers want to know "which field changed here", but with repeating groups it can be an entire row at once. There is also no guarantee there won't be changes in app-frontend-react causing multiple values to change at the same time. Also, what if you have a spotty internet connection and some requests to save will fail, some will be delayed - that might make the backend miss the event it was looking for. Relying on getting the specific component ID that made the change, and trusting you'll always get that event, I think it's a recipe for subtle bugs in the future. The "proper" way (I think) should be to use a diffing tool in the backend to generate a list of what changed from before, and we already have the tools for that there.

Most definitely see your point. These headers I think should be looked at as a "best effort" at telling the backend what was the cause of this put request, and might not be something we even document out a supported "feature". This solves a need now, and I agree some diffing tool would be better. But that requires more work. And more testing. And more time.

@nkylstad do you have have any thoughts on this? I do think @olemartinorg has a great point here. Since BRG technically does not go in production until 1. january, I guess they can live with some extra requests while we implement a better way to tell developers what changed "the proper way".

Or, should we merge this and leave it undocumented (or documented with a disclaimer)? Use at your own risk kind of thing. Any ideas, anyone?

@olemartinorg
Copy link
Contributor

I agree that documenting this with a disclaimer (or not documenting it) will work. When there's a pressing need, this works better than nothing, I guess.

@DanRJ: I seem to remember something about you using our JSON diff tool in OED to figure out which data fields have changed in ProcessDataWrite? My memories of the exact code is fuzzy, but was that something that could easily be re-used or documented as a suggested way to solve the problem of "I want to know what just changed"?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

83.3% 83.3% Coverage
0.0% 0.0% Duplication

@lorang92
Copy link
Contributor Author

A diff might be something we should look into in the future as a good alternative. Might need some other changes in the backend for that to be viable. I'm merging this, seems harmless and solves real-world issues.

@lorang92 lorang92 merged commit 0cf683a into main Sep 30, 2022
@lorang92 lorang92 deleted the feature/info-data-write branch September 30, 2022 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants