Skip to content

Conversation

@alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented Aug 29, 2025

Part of getodk/central#1315: aiming to reduce chained requests for already-authenticated returning web users by one.

What has been done to verify that this works as intended?

A couple of new tests!

Why is this the best possible solution? Were any other approaches considered?

  • could include in non-extended response, but it looks like frontend requests the extended response
  • should /sessions/restore be documented as deprecated?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Should make frontend a little faster for them sometimes.

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

Docs have been updated.

Before submitting this PR, please make sure you have:

  • run make test and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

@alxndrsn alxndrsn requested a review from matthew-white August 29, 2025 06:00
Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

Looks great!

@matthew-white
Copy link
Member

matthew-white commented Aug 29, 2025

should /sessions/restore be documented as deprecated?

I don't think we even document that endpoint actually. I think we could remove the endpoint entirely once we've stopped using it in Frontend.

@matthew-white
Copy link
Member

I don't think we even document that endpoint actually.

I think the reason we didn't document it is because it was originally introduced to support cookie auth. We made the decision not to document the details of cookie auth.

@matthew-white
Copy link
Member

@alxndrsn, do you think we could go ahead and merge this PR? I'm confident that I'll be able to use this change on the frontend, but I probably won't get to it until I've made more progress on maps.

@alxndrsn
Copy link
Contributor Author

alxndrsn commented Sep 4, 2025

I probably won't get to it until I've made more progress on maps

I'm happy to hold off merging until frontend code is ready.

@matthew-white
Copy link
Member

Sorry, I don't think I'm going to get to the frontend part of this before regression testing. Too much mapping to do! I'll plan to work on that at the start of v2025.4 so that we can get this PR merged soon.

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.

2 participants