Fix initial value of user in WebComponentLoader#1021
Merged
floehopper merged 4 commits intomainfrom May 28, 2024
Merged
Conversation
floehopper
force-pushed
the
fix-user-in-web-component-loader
branch
from
c9c3e91 to
afad6b9
Compare
floehopper
temporarily deployed
to
previews/fix-user-in-web-component-loader
GitHub Actions
Inactive
floehopper
force-pushed
the
fix-user-in-web-component-loader
branch
from
afad6b9 to
6c5fc18
Compare
floehopper
temporarily deployed
to
previews/fix-user-in-web-component-loader
GitHub Actions
Inactive
Since we're viewing the web component page in this spec, I think it's clearer to talk about viewing the project rather than visiting the project's URL which only really makes sense in e.g. editor-standalone.
I'm planning to add checks when viewing the remixed project. This will make that easier.
Previously, even if the `authKey` property was provided and a logged-in user was stored in `localStorage`, on first render the value of `user` was `null`. This was because the `localStorageUserMiddleware` which dispatches `setUser` to store the user in Redux had not yet been triggered. This meant that previouly if we tried to use the web component to view a project that the logged-in user owned, the request to editor-api to load the project would respond with a 403 Forbidden and the project would not load. I'm fairly (but not completely) sure that the assertions in the "with user set in local storage" group of examples in `src/containers/WebComponentLoader.test.js` were incorrect, i.e. if the `authKey` is set and a `user` is present in `localStorage` we ought to load the project based on the fact the user is logged in. I have fixed these assertions. The `null` value of the `user` property in the call to `useProjectPersistence` in the "When no user is in state with no user in local storage" group of examples in `src/containers/WebComponentLoader.test.js` is due to the return value for `localStorage.getItem(authKey)` and not the fallback in the new ternary expression in `WebComponentLoader`. I think this is fine. I'm not sure whether my fix is definitely correct/idiomatic - maybe I should be using a `useState` hook for the `localStorageUser` in combination with a `useEffect` hook...? Also stepping back a bit, it's not entirely clear to me that dispatching `setUser` [1] in `localStorageUserMiddleware` is definitely the most sensible place to do that. [1]: https://github.com/RaspberryPiFoundation/editor-ui/blob/92b45d59d8187b26538c03af825bb34313b7e344/src/redux/middlewares/localStorageUserMiddleware.js#L11
|
floehopper
force-pushed
the
fix-user-in-web-component-loader
branch
from
6c5fc18 to
4f2d41f
Compare
|
danhalson
reviewed
May 28, 2024
danhalson
approved these changes
May 28, 2024
Contributor
danhalson
left a comment
danhalson
left a comment
There was a problem hiding this comment.
looks great..I do agree about dispatching setUser in middleware, but I'm not sure we need to address that now?
Merged
floehopper
added a commit
that referenced
this pull request
Jun 4, 2024
### Added - Add `project_name_editable` attribute to web component (#1009) - Fires custom event when the theme changes (#1015) - Add `output_only` attribute to web component (#1019 & originally #782) - Add `assets_identifier` attribute to web component (#1019 & originally #901) - Enhance `code` attribute on web component to override project main component content (#1019 & originally #901) - Add `runCode`, `stopCode` & `rerunCode` methods to web component (#1019 & originally #899) - Send error details in "editor-runCompleted" event (#1019 & originally #915) - Return error details to web component (#1019 & originally #915) - Add `output_panels` attribute to web component (#1019 & originally #909) ### Changed - Remove unused `/embedded/projects/:identifier` route (#1013) ### Fixed - Remove unused `REACT_APP_LOGIN_ENABLED` env var (#1006) - Fix infinite remix loop when `BYPASS_AUTH` set in `editor-api` (#1007) - Fixes for docker-compose.yml (#1008) - Fix deprecation warnings in GitHub Actions (#1011) - Removed unused `isEmbedded` param from `useProject` call in `EmbeddedViewer` (#1016) - Improvements to Cypress specs in CI (#1017) - Fix warnings and verbose output when starting Webpack Dev Server (#1018) - Add e2e spec for project remix behaviour in web component (#1020) - Fix initial value of `user` in `WebComponentLoader` (#1021) - Make `authKey` in e2e web component spec more realistic (#1022) - Remove unused `ComponentStore` (#1023)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, even if the
authKeyproperty was provided and a logged-in user was stored inlocalStorage, on first render the value ofuserwasnull. This was because thelocalStorageUserMiddlewarewhich dispatchessetUserto store the user in Redux had not yet been triggered.This meant that previously if we tried to use the web component to view a project that the logged-in user owned, the request to editor-api to load the project would respond with a 403 Forbidden and the project would not load.
I'm fairly (but not completely) sure that the assertions in the "with user set in local storage" group of examples in
src/containers/WebComponentLoader.test.jswere incorrect, i.e. if theauthKeyis set and auseris present inlocalStoragewe ought to load the project based on the fact the user is logged in. I have fixed these assertions.The
nullvalue of theuserproperty in the call touseProjectPersistencein the "When no user is in state with no user in local storage" group of examples insrc/containers/WebComponentLoader.test.jsis due to the return value forlocalStorage.getItem(authKey)and not the fallback in the new ternary expression inWebComponentLoader. I think this is fine.I'm not sure whether my fix is definitely correct/idiomatic - maybe I should be using a
useStatehook for thelocalStorageUserin combination with auseEffecthook...?Also stepping back a bit, it's not entirely clear to me that dispatching
setUserinlocalStorageUserMiddlewareis definitely the most sensible place to do that.