Skip to content
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

Add Content for User page, preload user & throw 404 accordingly #1901

Merged
merged 7 commits into from
Jan 22, 2020

Conversation

dsevillamartin
Copy link
Member

Fixes #1846

Changes proposed in this pull request:

  • Preload user
    • Tested it works correctly, API call to /api/users/ no longer needed as preloadedApiDocument() call adds it to the forum payload
  • Return 404 if user is not found

Reviewers should focus on:
Doesn't add a no-JS view. I assume we want one, not sure about what items to include though. Posts/discussions would be hard, and require backend handling of those routes as well.

Screenshot
image

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

src/Forum/Content/User.php Outdated Show resolved Hide resolved
src/Forum/Content/User.php Outdated Show resolved Hide resolved
js/src/forum/components/UserPage.js Show resolved Hide resolved
src/Forum/Content/User.php Show resolved Hide resolved
@dsevillamartin dsevillamartin force-pushed the ds/1846-preload-user-data branch from 5ffa330 to 06e7871 Compare January 18, 2020 18:10
[ci skip] [skip ci]
@dsevillamartin
Copy link
Member Author

dsevillamartin commented Jan 18, 2020

Applied some changes and added a check for user ID instead of just the username when preloading.

Do we want to throw ModelNotFoundException instead of RouteNotFoundException? I feel like the current implementation could be confusing, as the users route exists but the user model specified doesn't. And the API endpoint throws the ModelNotFoundException, so we should probably be consistent there.

@franzliedke
Copy link
Contributor

Agreed. The model is not found, so we should use that exception.

@dsevillamartin
Copy link
Member Author

@franzliedke Switched exception thrown 👍

Copy link
Contributor

@franzliedke franzliedke left a comment

Choose a reason for hiding this comment

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

Beautiful, thank you!

@franzliedke franzliedke merged commit 02899d4 into master Jan 22, 2020
@franzliedke franzliedke deleted the ds/1846-preload-user-data branch January 22, 2020 23:01
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.

Invalid/Not existed username handle in API ShowUserController
2 participants