fix: add authenticatedpageroute so learner can create a shared record link #187
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #187 +/- ##
=======================================
Coverage 69.60% 69.60%
=======================================
Files 27 27
Lines 408 408
Branches 90 90
=======================================
Hits 284 284
Misses 123 123
Partials 1 1
☔ View full report in Codecov by Sentry. |
| @@ -29,7 +29,7 @@ subscribe(APP_READY, () => { | |||
| <Routes> | |||
| <Route | |||
| path={ROUTES.PROGRAM_RECORDS} | |||
| element={<ProgramRecordsList />} | |||
| element={<AuthenticatedPageRoute><ProgramRecordsList /></AuthenticatedPageRoute>} | |||
There was a problem hiding this comment.
I was under the impression that the AuthenticatedPageRoute component was actually a react-router Route of its own. This is indicated in the docs, HOWEVER, the implementation used in frontend-platform test files just uses it as a wrapper (like this PR does).
When digging into the code, I don't see anywhere that AuthenticatedPageRoute would ever return an actual Route component. See the code in frontend-platform.
What's strange is that there ARE implementations where MFEs are adding props like path and component to the AuthenticatedPageRoute component. See here in frontend-app-learner-portal-enterprise.
All of this to say, this PR is clearly fine but I'm also clearly a little confused so will be bringing this to fedX group to hopefully get some clarification on how this component is intended to be used.
There was a problem hiding this comment.
Hey thanks for bringing in all of these PR examples! Very valid point in terms of implementation. Originally I did try replacing the Route element with a AuthenticatedPageRoute, but then an error appeared:
[AuthenticatedPageRoute] is not a <Route> component. All component children of <Routes> must be a <Route> or <React.Fragment>
This must be a hard requirement for frontend-platform v5, and previous PRs might need to be updated to reflect this rule.
There was a problem hiding this comment.
One other thing to note is that the comments in the docs is from 4 years ago (found via git blame), so it should be updated to better represent its implementation.
There was a problem hiding this comment.
And lastly, it looks like the implementation in frontend-app-learner-portal-enterprise has placed the routes directly into App.jsx (and not wrapped with a Routes element), and this was done over two years ago. And they're still using frontend-platform v4.
So from that, I think that AuthenticatedPageRoute once had served as a substitute for Route, but it no longer does in v5.
Description:
Resolves the bug APER-2606: Learner unable to create public record without reauthenticating
When the learner attempted to create a public record link, their clipboard copied the private URL and they weren't able to create a public URL unless they re-authenticated. This PR wraps the
ProgramRecordsList,ProgramCertificatesList, and the private version ofProgramRecordwithAuthenticatedPageRoute, which allows for the user to experience a redirect "to the login page when the route becomes active and the user is not authenticated" source.Note that the public view of
ProgramRecordis not wrapped in theAuthenticatedPageRoute. This route needs to not be wrapped so that non-learners are able to access the public record via shared URL.