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

Return NoContent for missing (optional) files #174

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

ivarne
Copy link
Member

@ivarne ivarne commented Jan 27, 2023

RuleConfiguration.js and RuleHandler.js will soon be deprecated and removed from the app template. Frontend still needs to poll for the files for backwards compatibility, but showing permanent 404 in the browser console is a bad developer experience.

This suggest to change the return code to 204 no content, but it should not be released in backend before a released version of frontend is tested and verified to support this new behaviour. It seems to work fine as is.

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

@ivarne ivarne marked this pull request as ready for review January 27, 2023 20:37
@tjololo tjololo added external-contribution-❤️ Pull request from a developer outside the Altinn teams. feature Label Pull requests with new features. Used when generation releasenotes labels Jan 30, 2023
@RonnyB71
Copy link
Member

We had a discussion about this yesterday and feel that the 404 is the correct status for a file that is requested from frontend and should be there. Also the "will soon be deprecated" is something we need to discuss as we can't deprecated it until we have alternative solutions for custom code.

@ivarne
Copy link
Member Author

ivarne commented Jan 31, 2023

Why should RuleConfiguration.json and RuleHandler.js be handled differently? 404 is tracked by application insights as an error, and an app that is missing RuleHandler is definitely not erronious.

@RonnyB71
Copy link
Member

Valid point and probably they shouldn't be treated differently. The reason why the are is probably historical and of no value.

We did a quick re-run of yesterdays discussion and realized we didn't consider the optional aspects of these files. Only that a file was requested and it doesn't exists. So the api purists in us won:-D Some of the config files should definitely return 404 as it is an error if they don't exists. These two however, are optional might very well be left blank as well.

Approving now, and thx for continuously challenging:)

Copy link
Member

@RonnyB71 RonnyB71 left a comment

Choose a reason for hiding this comment

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

🕹️

@RonnyB71 RonnyB71 changed the title Return NoContent for missing (soon to be optional) files Return NoContent for missing (optional) files Jan 31, 2023
@RonnyB71 RonnyB71 merged commit ecd5069 into Altinn:main Jan 31, 2023
@ivarne ivarne deleted the NoContentOptional branch January 31, 2023 10:16
@ivarne
Copy link
Member Author

ivarne commented Jan 31, 2023

I'm actually not against the purist opinion, if that would include giving frontend an alternative to just calling the API and look at the response when it wants to know if the old dynamics is used. Maybe a {org}/{app}/features endpoint that returns a set of feature flags.

@olemartinorg
Copy link
Contributor

I've been looking into something of that sort when implementing this issue:

To be more specific, it requests {org}/{app}/api/v1/featureset, which currently fails with a 404 on existing apps. However, it is expected (for that task) that the endpoint is created at some point, and returns a list of features supported on that backend version. Currently, the only recognized feature there is 'multipart saving' which allows for a multipart request for each data model save operation that includes details about which parts have changed since last time (along with their previous values).

@ivarne
Copy link
Member Author

ivarne commented Feb 1, 2023

Nice! You should probably add the PR to app-lib-dotnet for the endpoint to answer [] with 200 OK as soon as possible, so that logs don't fill up with 404 when the functionality actually launches in frontend. Then you can change it to ["multipart saving"] when you have a backend that supports it.

tjololo added a commit to Altinn/app-template-dotnet that referenced this pull request Apr 25, 2023
tjololo added a commit to Altinn/app-template-dotnet that referenced this pull request Apr 28, 2023
* Update nuget non-major dependencies

* GetRuleConfiguration now returns NoContent after Altinn/app-lib-dotnet#174 was merged

* maxSize in applicationmetadata.json is now repected after Altinn/app-lib-dotnet#121 was merged. Updated testdata

* PdfService extended with new PDFClient. Tests not updated to test new service

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Vemund Gaukstad <tjololo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contribution-❤️ Pull request from a developer outside the Altinn teams. feature Label Pull requests with new features. Used when generation releasenotes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants