-
-
Notifications
You must be signed in to change notification settings - Fork 964
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
fix: load return_to and append to errors #2333
fix: load return_to and append to errors #2333
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2333 +/- ##
==========================================
- Coverage 76.68% 76.64% -0.04%
==========================================
Files 318 318
Lines 17368 17211 -157
==========================================
- Hits 13319 13192 -127
+ Misses 3109 3087 -22
+ Partials 940 932 -8
Continue to review full report at Codecov.
|
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.
Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)
Thanks for your feedback @aeneasr - good suggestions all round 😄 I'll work on the suggested improvements and update the PR |
Could you please resolve the CI issues? :) https://github.com/ory/kratos/runs/5676532411?check_suite_focus=true#step:13:41 |
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've updated the PR using pop's callbacks which is a bit more save as they will always run :)
As noted in one of the comments, could you pelase add tests for these? A good indicator of whether tests are effective is to see if they pass when you comment your fixes. If they do pass there as well, they are not effective (as they do not test your changes but something else).
Hello @jacoblehr |
This merge request fixes the way that return_to urls are set on the self service flow objects; previously, this was only being set when marshalling the flow to JSON; I have added a call to set this when the flow is retrieved from the database. This means that the return_to can be appended to each of the error responses in the event that the flow is expired. There is an existing pull request that fixes this for the verification flow (I'm actually a colleague of the original submitter MilesNorton) but I think it's better in this case to fix all of these at once.
Related pull requests
Related issue(s)
return_to
on recovery is not being returned with the settings flow after the recovery email has been clicked #2285Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments
N/A