fix: improve disabled-account feedback and skill detail polish (#70)

* fix: keep download counts consistent across skill pages

* fix: stabilize empty search ordering across sorts

* fix: show disabled-account reason on login redirect

* fix: mute report input placeholder text

* fix: return skill detail to my skills page

* test: stabilize auth context filter coverage
This commit is contained in:
yun-zhi-ztl 2026-03-17 19:55:44 +08:00 committed by GitHub
parent 09e1933f61
commit cc5fc7586f
17 changed files with 173 additions and 44 deletions

View file

@ -1,10 +1,12 @@
package com.iflytek.skillhub.filter;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.iflytek.skillhub.auth.rbac.PlatformPrincipal;
import com.iflytek.skillhub.domain.namespace.NamespaceMember;
import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository;
import com.iflytek.skillhub.domain.namespace.NamespaceRole;
import com.iflytek.skillhub.domain.user.UserAccountRepository;
import com.iflytek.skillhub.dto.ApiResponseFactory;
import jakarta.servlet.FilterChain;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletRequest;
@ -14,6 +16,7 @@ import java.io.IOException;
import java.util.Map;
import java.util.stream.Collectors;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.http.MediaType;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.web.context.HttpSessionSecurityContextRepository;
@ -25,13 +28,19 @@ public class AuthContextFilter extends OncePerRequestFilter {
private final NamespaceMemberRepository namespaceMemberRepository;
private final UserAccountRepository userAccountRepository;
private final ApiResponseFactory apiResponseFactory;
private final ObjectMapper objectMapper;
private final boolean enforceActiveUserCheck;
public AuthContextFilter(NamespaceMemberRepository namespaceMemberRepository,
UserAccountRepository userAccountRepository,
ApiResponseFactory apiResponseFactory,
ObjectMapper objectMapper,
@Value("${skillhub.auth.enforce-active-user-check:true}") boolean enforceActiveUserCheck) {
this.namespaceMemberRepository = namespaceMemberRepository;
this.userAccountRepository = userAccountRepository;
this.apiResponseFactory = apiResponseFactory;
this.objectMapper = objectMapper;
this.enforceActiveUserCheck = enforceActiveUserCheck;
}
@ -44,7 +53,12 @@ public class AuthContextFilter extends OncePerRequestFilter {
if (principal != null) {
if (isInactiveUser(principal.userId())) {
clearAuthentication(request);
response.sendError(HttpServletResponse.SC_UNAUTHORIZED);
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
response.setContentType(MediaType.APPLICATION_JSON_VALUE);
objectMapper.writeValue(
response.getOutputStream(),
apiResponseFactory.error(HttpServletResponse.SC_UNAUTHORIZED, "error.auth.local.accountDisabled")
);
return;
}
request.setAttribute("userId", principal.userId());

View file

@ -1,5 +1,7 @@
package com.iflytek.skillhub.filter;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
import com.iflytek.skillhub.auth.rbac.PlatformPrincipal;
import com.iflytek.skillhub.domain.namespace.NamespaceMember;
import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository;
@ -7,17 +9,20 @@ import com.iflytek.skillhub.domain.namespace.NamespaceRole;
import com.iflytek.skillhub.domain.user.UserAccount;
import com.iflytek.skillhub.domain.user.UserAccountRepository;
import com.iflytek.skillhub.domain.user.UserStatus;
import com.iflytek.skillhub.dto.ApiResponseFactory;
import jakarta.servlet.FilterChain;
import jakarta.servlet.http.HttpSession;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.mock.web.MockHttpSession;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.core.context.SecurityContextHolder;
import java.util.List;
import java.util.Set;
import org.springframework.context.support.StaticMessageSource;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
@ -31,7 +36,20 @@ class AuthContextFilterTest {
private final NamespaceMemberRepository namespaceMemberRepository = mock(NamespaceMemberRepository.class);
private final UserAccountRepository userAccountRepository = mock(UserAccountRepository.class);
private final AuthContextFilter filter = new AuthContextFilter(namespaceMemberRepository, userAccountRepository, true);
private final AuthContextFilter filter;
AuthContextFilterTest() {
StaticMessageSource messageSource = new StaticMessageSource();
messageSource.addMessage("error.auth.local.accountDisabled", Locale.ENGLISH, "This account has been disabled");
ApiResponseFactory apiResponseFactory = new ApiResponseFactory(messageSource);
filter = new AuthContextFilter(
namespaceMemberRepository,
userAccountRepository,
apiResponseFactory,
new ObjectMapper().registerModule(new JavaTimeModule()),
true
);
}
@AfterEach
void clearSecurityContext() {
@ -45,7 +63,7 @@ class AuthContextFilterTest {
user.setStatus(UserStatus.DISABLED);
MockHttpServletRequest request = new MockHttpServletRequest();
HttpSession session = request.getSession(true);
MockHttpSession session = (MockHttpSession) request.getSession(true);
session.setAttribute("platformPrincipal", principal);
SecurityContextHolder.getContext().setAuthentication(
new UsernamePasswordAuthenticationToken(principal, null, List.of())
@ -59,7 +77,8 @@ class AuthContextFilterTest {
filter.doFilter(request, response, filterChain);
assertEquals(401, response.getStatus());
assertTrue(!request.isRequestedSessionIdValid() || request.getSession(false) == null);
assertTrue(response.getContentAsString().contains("\"code\":401"));
assertTrue(session.isInvalid());
assertNull(SecurityContextHolder.getContext().getAuthentication());
verify(filterChain, never()).doFilter(request, response);
}

View file

@ -154,7 +154,7 @@ public class SkillDownloadService {
result = buildBundleFromFiles(skill, version);
}
// Publish download event
skillRepository.incrementDownloadCount(skill.getId());
eventPublisher.publishEvent(new SkillDownloadedEvent(skill.getId(), version.getId()));
return result;
}

View file

@ -108,6 +108,7 @@ class SkillDownloadServiceTest {
assertEquals("Test Skill-1.0.0.zip", result.filename());
assertEquals(1000L, result.contentLength());
assertNotNull(result.content());
verify(skillRepository).incrementDownloadCount(1L);
verify(eventPublisher).publishEvent(any(SkillDownloadedEvent.class));
}
@ -151,6 +152,7 @@ class SkillDownloadServiceTest {
assertNotNull(result);
assertEquals("Test Skill-1.0.0.zip", result.filename());
assertNotNull(result.content());
verify(skillRepository).incrementDownloadCount(1L);
verify(eventPublisher).publishEvent(any(SkillDownloadedEvent.class));
}
@ -261,6 +263,7 @@ class SkillDownloadServiceTest {
assertEquals("test", output.toString());
}
verify(skillRepository).incrementDownloadCount(1L);
verify(eventPublisher).publishEvent(any(SkillDownloadedEvent.class));
}

View file

@ -1,23 +0,0 @@
package com.iflytek.skillhub.search.event;
import com.iflytek.skillhub.domain.event.SkillDownloadedEvent;
import com.iflytek.skillhub.domain.skill.SkillRepository;
import org.springframework.context.event.EventListener;
import org.springframework.scheduling.annotation.Async;
import org.springframework.stereotype.Component;
@Component
public class DownloadCountEventListener {
private final SkillRepository skillRepository;
public DownloadCountEventListener(SkillRepository skillRepository) {
this.skillRepository = skillRepository;
}
@EventListener
@Async("skillhubEventExecutor")
public void onSkillDownloaded(SkillDownloadedEvent event) {
skillRepository.incrementDownloadCount(event.skillId());
}
}

View file

@ -122,11 +122,13 @@ public class PostgresFullTextQueryService implements SearchQueryService {
// Sorting
if ("downloads".equals(query.sortBy())) {
sql.append("ORDER BY (SELECT download_count FROM skill WHERE id = skill_id) DESC ");
sql.append("ORDER BY (SELECT download_count FROM skill WHERE id = skill_id) DESC, ");
sql.append("(SELECT updated_at FROM skill WHERE id = skill_id) DESC, skill_id DESC ");
} else if ("rating".equals(query.sortBy())) {
sql.append("ORDER BY (SELECT rating_avg FROM skill WHERE id = skill_id) DESC ");
sql.append("ORDER BY (SELECT rating_avg FROM skill WHERE id = skill_id) DESC, ");
sql.append("(SELECT updated_at FROM skill WHERE id = skill_id) DESC, skill_id DESC ");
} else if ("newest".equals(query.sortBy())) {
sql.append("ORDER BY (SELECT updated_at FROM skill WHERE id = skill_id) DESC ");
sql.append("ORDER BY (SELECT updated_at FROM skill WHERE id = skill_id) DESC, skill_id DESC ");
} else if (useRelevanceOrdering) {
sql.append("ORDER BY CASE ");
sql.append("WHEN ").append(TITLE_SQL).append(" = :titleExact THEN 4 ");
@ -135,14 +137,14 @@ public class PostgresFullTextQueryService implements SearchQueryService {
sql.append("ELSE 1 END DESC, ");
if (useShortPrefixTitleSearch) {
sql.append("ts_rank_cd(").append(TITLE_VECTOR_SQL)
.append(", to_tsquery('simple', :tsQuery)) DESC, updated_at DESC ");
.append(", to_tsquery('simple', :tsQuery)) DESC, updated_at DESC, skill_id DESC ");
} else if (hasTsQuery) {
sql.append("ts_rank_cd(search_vector, to_tsquery('simple', :tsQuery)) DESC, updated_at DESC ");
sql.append("ts_rank_cd(search_vector, to_tsquery('simple', :tsQuery)) DESC, updated_at DESC, skill_id DESC ");
} else {
sql.append("updated_at DESC ");
sql.append("updated_at DESC, skill_id DESC ");
}
} else {
sql.append("ORDER BY updated_at DESC ");
sql.append("ORDER BY updated_at DESC, skill_id DESC ");
}
// Pagination

View file

@ -229,6 +229,41 @@ class PostgresFullTextQueryServiceTest {
verify(nativeQuery, never()).setParameter(org.mockito.ArgumentMatchers.eq("titleExact"), anyString());
verify(nativeQuery, never()).setParameter(org.mockito.ArgumentMatchers.eq("titlePrefix"), anyString());
verify(nativeQuery).setParameter("titleLike", "%51222222333%");
var sqlCaptor = org.mockito.ArgumentCaptor.forClass(String.class);
verify(entityManager, org.mockito.Mockito.times(2)).createNativeQuery(sqlCaptor.capture());
assertThat(sqlCaptor.getAllValues().getFirst())
.contains("ORDER BY (SELECT download_count FROM skill WHERE id = skill_id) DESC, (SELECT updated_at FROM skill WHERE id = skill_id) DESC, skill_id DESC");
}
@Test
void emptyKeywordRelevanceShouldUseStableNewestOrdering() {
EntityManager entityManager = mock(EntityManager.class);
Query nativeQuery = mock(Query.class);
Query countQuery = mock(Query.class);
when(entityManager.createNativeQuery(anyString()))
.thenReturn(nativeQuery)
.thenReturn(countQuery);
when(nativeQuery.setParameter(anyString(), org.mockito.ArgumentMatchers.any())).thenReturn(nativeQuery);
when(countQuery.setParameter(anyString(), org.mockito.ArgumentMatchers.any())).thenReturn(countQuery);
when(nativeQuery.getResultList()).thenReturn(List.of());
when(countQuery.getSingleResult()).thenReturn(0L);
PostgresFullTextQueryService service = new PostgresFullTextQueryService(entityManager);
service.search(new SearchQuery(
null,
null,
new SearchVisibilityScope(null, Set.of(), Set.of()),
"relevance",
0,
12
));
var sqlCaptor = org.mockito.ArgumentCaptor.forClass(String.class);
verify(entityManager, org.mockito.Mockito.times(2)).createNativeQuery(sqlCaptor.capture());
assertThat(sqlCaptor.getAllValues().getFirst())
.contains("ORDER BY updated_at DESC, skill_id DESC");
}
@Test

View file

@ -157,8 +157,9 @@ const skillsRoute = createRoute({
const loginRoute = createRoute({
getParentRoute: () => rootRoute,
path: 'login',
validateSearch: (search: Record<string, unknown>) => ({
validateSearch: (search: Record<string, unknown>): { returnTo: string; reason?: string } => ({
returnTo: typeof search.returnTo === 'string' ? search.returnTo : '',
reason: typeof search.reason === 'string' ? search.reason : undefined,
}),
component: LoginPage,
})
@ -207,6 +208,9 @@ const namespaceRoute = createRoute({
const skillDetailRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/space/$namespace/$slug',
validateSearch: (search: Record<string, unknown>): { returnTo?: string } => ({
returnTo: typeof search.returnTo === 'string' && search.returnTo.startsWith('/') ? search.returnTo : undefined,
}),
component: SkillDetailPage,
})

View file

@ -41,7 +41,10 @@ export function MySkillsPage() {
const submitPromotionMutation = useSubmitPromotion()
const handleSkillClick = (namespace: string, slug: string) => {
navigate({ to: `/space/${namespace}/${slug}` })
navigate({
to: `/space/${namespace}/${slug}`,
search: { returnTo: '/dashboard/skills' },
})
}
const resolveStatusLabel = (status?: string) => {

View file

@ -25,6 +25,7 @@ export function LoginPage() {
const { data: authMethods } = useAuthMethods(search.returnTo)
const returnTo = search.returnTo && search.returnTo.startsWith('/') ? search.returnTo : '/dashboard'
const disabledMessage = search.reason === 'accountDisabled' ? t('apiError.auth.accountDisabled') : null
const directMethod = directAuthConfig.provider
? authMethods?.find((method) =>
method.methodType === 'DIRECT_PASSWORD' && method.provider === directAuthConfig.provider)
@ -71,6 +72,11 @@ export function LoginPage() {
<div className="glass-strong p-8 rounded-2xl">
<div className="space-y-6">
{disabledMessage ? (
<div className="rounded-xl border border-red-200 bg-red-50 px-4 py-3 text-sm text-red-700">
{disabledMessage}
</div>
) : null}
<SessionBootstrapEntry
methodDisplayName={bootstrapMethod?.displayName}
onAuthenticated={() => navigate({ to: returnTo })}

View file

@ -1,6 +1,6 @@
import { useState } from 'react'
import { useTranslation } from 'react-i18next'
import { useParams, useNavigate, useRouterState } from '@tanstack/react-router'
import { useParams, useNavigate, useRouterState, useSearch } from '@tanstack/react-router'
import { useMutation, useQueryClient } from '@tanstack/react-query'
import { ArrowLeft } from 'lucide-react'
import { MarkdownRenderer } from '@/features/skill/markdown-renderer'
@ -14,7 +14,7 @@ import { adminApi, ApiError, buildApiUrl, WEB_API_PREFIX } from '@/api/client'
import { useSubmitSkillReport } from '@/features/report/use-skill-reports'
import { formatLocalDateTime } from '@/shared/lib/date-time'
import { incrementSkillDownloadCount } from '@/shared/lib/skill-download-cache'
import { getSkillSquareSearch } from '@/shared/lib/skill-navigation'
import { getSkillSquareSearch, normalizeSkillDetailReturnTo } from '@/shared/lib/skill-navigation'
import { formatCompactCount } from '@/shared/lib/number-format'
import { resolveDocumentationFilePath } from '@/shared/lib/skill-documentation'
import { NamespaceBadge } from '@/shared/components/namespace-badge'
@ -75,6 +75,7 @@ export function SkillDetailPage() {
const { t, i18n } = useTranslation()
const navigate = useNavigate()
const location = useRouterState({ select: (s) => s.location })
const search = useSearch({ from: '/space/$namespace/$slug' })
const queryClient = useQueryClient()
const [reportDialogOpen, setReportDialogOpen] = useState(false)
const [reportReason, setReportReason] = useState('')
@ -211,6 +212,11 @@ export function SkillDetailPage() {
}
const handleBack = () => {
const returnTo = normalizeSkillDetailReturnTo(search.returnTo)
if (returnTo) {
navigate({ to: returnTo })
return
}
navigate({ to: '/search', search: getSkillSquareSearch() })
}

View file

@ -37,6 +37,15 @@ describe('handleApiError', () => {
expect(window.location.href).toBe('/login')
})
it('preserves disabled-account reason when redirecting to login', async () => {
const { ApiError, handleApiError } = await import('./api-error')
handleApiError(new ApiError('This account has been disabled', 401, 'This account has been disabled'))
expect(errorSpy).not.toHaveBeenCalled()
expect(window.location.href).toBe('/login?reason=accountDisabled')
})
it('falls back to the server message for non-standard api errors', async () => {
const { ApiError, handleApiError } = await import('./api-error')

View file

@ -1,6 +1,8 @@
import i18n from '@/i18n/config'
import { toast } from './toast'
const ACCOUNT_DISABLED_REASON = 'accountDisabled'
function resolveLocalizedMessage(message?: string): string | undefined {
if (!message) {
return undefined
@ -23,6 +25,25 @@ export class ApiError extends Error {
}
}
function isAccountDisabledError(error: ApiError): boolean {
const accountDisabledMessages = [
i18n.t('apiError.auth.accountDisabled'),
i18n.getFixedT('en')('apiError.auth.accountDisabled'),
i18n.getFixedT('zh')('apiError.auth.accountDisabled'),
]
const normalizedServerMessage = (error.serverMessage ?? '').toLowerCase()
const normalizedMessage = error.message.toLowerCase()
return error.serverMessageKey === 'error.auth.local.accountDisabled'
|| error.serverMessage === 'error.auth.local.accountDisabled'
|| accountDisabledMessages.includes(error.serverMessage ?? '')
|| accountDisabledMessages.includes(error.message)
|| normalizedServerMessage.includes('disabled')
|| normalizedMessage.includes('disabled')
|| (error.serverMessage ?? '').includes('禁用')
|| error.message.includes('禁用')
}
export function handleApiError(error: unknown): void {
if (!(error instanceof ApiError)) {
toast.error(i18n.t('apiError.unknown'))
@ -32,6 +53,10 @@ export function handleApiError(error: unknown): void {
const { status } = error
if (status === 401) {
if (isAccountDisabledError(error)) {
window.location.href = `/login?reason=${ACCOUNT_DISABLED_REASON}`
return
}
toast.error(i18n.t('apiError.unauthorized'))
window.location.href = '/login'
return

View file

@ -1,5 +1,5 @@
import { describe, expect, it } from 'vitest'
import { getSkillSquareSearch } from './skill-navigation'
import { getSkillSquareSearch, normalizeSkillDetailReturnTo } from './skill-navigation'
describe('getSkillSquareSearch', () => {
it('returns the default search params for the skill square', () => {
@ -11,3 +11,13 @@ describe('getSkillSquareSearch', () => {
})
})
})
describe('normalizeSkillDetailReturnTo', () => {
it('returns the provided dashboard route when coming from my skills', () => {
expect(normalizeSkillDetailReturnTo('/dashboard/skills')).toBe('/dashboard/skills')
})
it('drops invalid return targets', () => {
expect(normalizeSkillDetailReturnTo('https://example.com/elsewhere')).toBeUndefined()
})
})

View file

@ -6,3 +6,7 @@ export function getSkillSquareSearch() {
starredOnly: false,
}
}
export function normalizeSkillDetailReturnTo(returnTo?: string) {
return returnTo && returnTo.startsWith('/') ? returnTo : undefined
}

View file

@ -0,0 +1,9 @@
import { describe, expect, it } from 'vitest'
import { INPUT_BASE_CLASS_NAME } from './input'
describe('INPUT_BASE_CLASS_NAME', () => {
it('uses muted placeholder styling', () => {
expect(INPUT_BASE_CLASS_NAME).toContain('placeholder:text-muted-foreground')
expect(INPUT_BASE_CLASS_NAME).not.toContain('placeholder:text-[var(--text-placeholder)]')
})
})

View file

@ -3,13 +3,16 @@ import { cn } from '@/shared/lib/utils'
export interface InputProps extends React.InputHTMLAttributes<HTMLInputElement> {}
export const INPUT_BASE_CLASS_NAME =
'flex h-11 w-full rounded-lg border bg-white px-4 py-2 text-sm text-foreground ring-offset-background transition-all duration-200 file:border-0 file:bg-transparent file:text-sm file:font-medium placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-primary/40 focus-visible:border-primary/50 disabled:cursor-not-allowed disabled:opacity-50'
const Input = React.forwardRef<HTMLInputElement, InputProps>(
({ className, type, style, ...props }, ref) => {
return (
<input
type={type}
className={cn(
'flex h-11 w-full rounded-lg border bg-white px-4 py-2 text-sm text-foreground ring-offset-background transition-all duration-200 file:border-0 file:bg-transparent file:text-sm file:font-medium placeholder:text-[var(--text-placeholder)] focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-primary/40 focus-visible:border-primary/50 disabled:cursor-not-allowed disabled:opacity-50',
INPUT_BASE_CLASS_NAME,
className
)}
style={{ borderColor: 'hsl(var(--border))', ...style }}