fix: more robust redirects for verification to result page #339
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the email verification and account deletion flows to use route parameters instead of TempData for passing success/error messages to the Result page. The main motivation is to fix redirect issues where users were being redirected back to the verification page instead of seeing the result.
Key changes:
- Replaced TempData-based message passing with route parameter-based approach
- Updated Result.cshtml to accept route parameters (
{action?}/{outcome?}) and use pattern matching to determine content - Refactored code structure from delegate syntax to local functions for improved readability
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| coffeecard/CoffeeCard.WebApi/Pages/VerifyEmail.cshtml.cs | Refactored to pass verification results via route parameters instead of TempData; simplified code structure using local function |
| coffeecard/CoffeeCard.WebApi/Pages/VerifyDelete.cshtml.cs | Refactored to pass deletion success via route parameters instead of TempData; simplified code structure using local function |
| coffeecard/CoffeeCard.WebApi/Pages/Result.cshtml | Updated to read action/outcome from route parameters and use pattern matching to display appropriate messages; removed TempData dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TTA777
left a comment
TTA777
left a comment
There was a problem hiding this comment.
Good initiative, but as the AI said, you have a problem where you are not accounting for the internals of the pageUtil still using TempData. You also missed the recover page, so that one is likely broken in this pr.
Personally, also not a huge fan of how the outcomes were handled, with there being a couple of "magic strings" per page. I would think an enum could do?
I've tried to address these concerns in #340. Have a look, and you can merge that into this one if you agree, or draw inspiration
Looks good. I agree the original was not optimal. I have merged your changes into this. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #339 +/- ##
=======================================
Coverage ? 13.89%
=======================================
Files ? 239
Lines ? 16956
Branches ? 259
=======================================
Hits ? 2356
Misses ? 14554
Partials ? 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Looks like the verification page has some issues with redirecting to the result page, resulting in being redirected back to the verification page (with the same token), thus displaying the error page. This is an attempt to make the redirect routing a bit more robust, but since it doesn't fail locally it's a bit difficult to test properly 🙃