Skip to content

馃悰 Fixed contributor profile editor navigation to posts page #25629

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rob-ghost merged 1 commit into from
Dec 9, 2025
Merged

馃悰 Fixed contributor profile editor navigation to posts page #25629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/admin-x-settings/src/components/settings/general/user-detail-modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ const UserDetailModalContent: React.FC<{user: User}> = ({user}) => {
if (canAccessSettings(currentUser)) {
updateRoute('staff');
} else {
updateRoute({isExternal: true, route: 'analytics'});
// Contributors can't access settings, exit to let the shell handle navigation
updateRoute({isExternal: true, route: ''});
}
}, [currentUser, updateRoute]);

Expand Down
10 changes: 3 additions & 7 deletions apps/admin-x-settings/src/main-content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const Page: React.FC<{children: ReactNode}> = ({children}) => {

const MainContent: React.FC = () => {
const {currentUser} = useGlobalData();
const {route, updateRoute, loadingModal} = useRouting();
const {loadingModal} = useRouting();
const {isDirty} = useGlobalDirtyState();

const navigateAway = (escLocation: string) => {
Expand Down Expand Up @@ -50,12 +50,8 @@ const MainContent: React.FC = () => {
toast.remove();
}, []);

useEffect(() => {
if (!canAccessSettings(currentUser) && route !== `staff/${currentUser.slug}`) {
updateRoute(`staff/${currentUser.slug}`);
}
}, [currentUser, route, updateRoute]);

// Contributors/Authors only see their profile modal (rendered via routing)
// Don't render the main settings content for them
if (!canAccessSettings(currentUser)) {
return null;
}
Expand Down
14 changes: 8 additions & 6 deletions apps/admin-x-settings/test/acceptance/permissions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,22 @@ test.describe('User permissions', async () => {
await expect(page.getByTestId('title-and-description')).toBeHidden();
});

// Note: Author/Contributor redirect to profile is handled by the Ember router (settings-x.js),
// not by the React app. These tests verify the UI renders correctly when on the profile route.
test('Authors can only see their own profile', async ({page}) => {
await mockApi({page, requests: {
...globalDataRequests,
browseMe: {...globalDataRequests.browseMe, response: meWithRole('Author')}
}});

await page.goto('/');
// Navigate directly to profile route using hash-based routing
// (Ember router handles redirect in production)
await page.goto('/#/settings/staff/owner');

await expect(page.getByTestId('user-detail-modal')).toBeVisible();
await expect(page.getByTestId('sidebar')).toBeHidden();
await expect(page.getByTestId('users')).toBeHidden();
await expect(page.getByTestId('title-and-description')).toBeHidden();

expect(page.url()).toMatch(/\/owner$/);
});

test('Contributors can only see their own profile', async ({page}) => {
Expand All @@ -38,13 +40,13 @@ test.describe('User permissions', async () => {
browseMe: {...globalDataRequests.browseMe, response: meWithRole('Contributor')}
}});

await page.goto('/');
// Navigate directly to profile route using hash-based routing
// (Ember router handles redirect in production)
await page.goto('/#/settings/staff/owner');

await expect(page.getByTestId('user-detail-modal')).toBeVisible();
await expect(page.getByTestId('sidebar')).toBeHidden();
await expect(page.getByTestId('users')).toBeHidden();
await expect(page.getByTestId('title-and-description')).toBeHidden();

expect(page.url()).toMatch(/\/owner$/);
});
Comment on lines 19 to 51

@kevinansfield kevinansfield Dec 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could these tests be improved? The assertions don't really match the naming which makes them difficult to reason about and potentially misleading.

If the expectation is that opening another user's profile directly in the settings app only renders the logged-in user's profile should we have some assertions that we're showing the right data rather than just that a profile modal is shown? Alternatively if these tests no longer make sense perhaps we should remove them?

@rob-ghost rob-ghost Dec 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They probably definitely can be improved, however I'd prefer to convert these to E2E tests as soon as we have the ability to assume different roles and delete this suite.

The changes in this PR are just to make them pass after we moved the redirect to the router.

In order to not block the React shell rollout, would you be happy to merge this fix as it stands and we can follow up with E2E tests in this ticket?

@kevinansfield kevinansfield Dec 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

馃憤 if we're getting rid of these tests when we add e2e tests for the profile screen then its not a blocker

});
16 changes: 16 additions & 0 deletions ghost/admin/app/routes/settings-x.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,22 @@ export default class SettingsXRoute extends AuthenticatedRoute {
@service ui;
@service modals;

beforeModel(transition) {
super.beforeModel(...arguments);

// Contributors and Authors can only access their own profile in settings
if (this.session.user.isAuthorOrContributor) {
// Check if they're trying to access their own profile route
const subPath = transition.to?.params?.sub;
const ownProfilePath = `staff/${this.session.user.slug}`;

// Only allow access to their own profile, redirect everything else
if (subPath !== ownProfilePath) {
return this.transitionTo('settings-x.settings-x', ownProfilePath);
}
}
}

activate() {
super.activate(...arguments);

Expand Down