Merge pull request #962 from iflytek/fix/issue-946-promotion-delete-20261015

fix(lifecycle): clarify deletion of promoted skills
This commit is contained in:
XiaoSeS 2026-10-16 12:38:17 +08:00 • committed by GitHub
commit a962a45c02
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 324 additions and 17 deletions

View file

@ -154,6 +154,18 @@ Status: contract reference with documented implementation exception
- 全局 Skill 也可独立上传并审核。提升更新与独立上传使用相同版本号时不能互相覆盖;不同号版本分别发布,latest 指向最后通过审核的版本,不按版本字符串大小排序。
- 撤销提升不同于归档或隐藏:平台审核通过后移除全局派生 Skill 的公众入口及记录,保留来源团队 Skill、提升和撤销审计,并保护共用对象存储文件。重新提升形成新的全局 Skill ID。
### 5.4 已提升技能如何删除
团队技能与提升生成的全局技能是两条关联记录。只撤回待审核版本、取消更新或撤回尚未批准的申请,**不会删除已经发布的全局技能,也不会解除首次提升的关联**。
若删除时提示须先撤销全局发布(旧版本提示“已提升的技能须通过审核撤销流程删除”),按以下顺序操作:
1. 打开**来源团队技能详情页**,在提升区域选择“申请撤销全局发布”,填写原因并提交。
2. 平台管理员在提升管理的撤销申请中审核;申请处于待审或被拒绝时,全局技能仍然存在,两条技能都不能绕过该流程直接删除。具有相应平台治理权限的管理员也可在来源详情页直接撤销。
3. 撤销批准后,全局技能及其地址自动删除,来源团队技能和版本仍保留。此时如需连来源一起删除,由技能所有者或超级管理员执行来源技能的永久删除。
撤销时已有的全局更新申请会失效,不能在全局记录删除后重新发布到该记录。提升撤销保留提升、撤销与删除审计;来源技能后续永久删除仍按既有物理删除流程清理其业务记录,操作审计继续保留。不要通过手工清除来源关联字段来绕过撤销审核。
## 6. 权限边界
- `withdraw-review`:仅提交人本人

View file

@ -290,7 +290,7 @@ promotion.revocation.target_changed=The global target changed; submit a new revo
promotion.revocation.duplicate_pending=A revocation request is already pending for this global skill
promotion.revocation.not_found=Revocation request {0} was not found
promotion.revocation.not_pending=Revocation request {0} is no longer pending
promotion.revocation.required=Use the reviewed revocation workflow to remove a promoted skill
promotion.revocation.required=Request global revocation from the source team skill details; delete the source after approval
promotion.revocation.page_invalid=Page must be nonnegative and size must be between 1 and 100
error.auth.settings.unavailable=Authentication settings are unavailable
error.system.roleGrant.unverifiedProvider=This identity provider does not attest verified email and cannot use initial role rules

View file

@ -290,7 +290,7 @@ promotion.revocation.target_changed=全局目标已经变化,请重新提交
promotion.revocation.duplicate_pending=该全局技能已有待审核的撤销申请
promotion.revocation.not_found=撤销申请 {0} 不存在
promotion.revocation.not_pending=撤销申请 {0} 已不在待审核状态
promotion.revocation.required=已提升的技能须通过审核撤销流程删除
promotion.revocation.required=请先在来源团队技能详情中申请撤销全局发布,审核通过后再删除来源技能
promotion.revocation.page_invalid=页码不能为负数,每页数量须在 1 到 100 之间
error.auth.settings.unavailable=认证配置暂时不可用
error.system.roleGrant.unverifiedProvider=该身份来源不提供已验证邮箱,不能用于首次角色授权规则

View file

@ -0,0 +1,186 @@
package com.iflytek.skillhub.repository;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.iflytek.skillhub.domain.audit.AuditLogService;
import com.iflytek.skillhub.domain.namespace.Namespace;
import com.iflytek.skillhub.domain.namespace.NamespaceType;
import com.iflytek.skillhub.domain.review.PromotionRequest;
import com.iflytek.skillhub.domain.review.PromotionRequestKind;
import com.iflytek.skillhub.domain.review.PromotionRevocationService;
import com.iflytek.skillhub.domain.review.ReviewTaskStatus;
import com.iflytek.skillhub.domain.security.SecurityScanService;
import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException;
import com.iflytek.skillhub.domain.skill.Skill;
import com.iflytek.skillhub.domain.skill.SkillVersion;
import com.iflytek.skillhub.domain.skill.SkillVersionStatus;
import com.iflytek.skillhub.domain.skill.SkillVisibility;
import com.iflytek.skillhub.domain.skill.service.SkillHardDeleteService;
import com.iflytek.skillhub.domain.skill.service.SkillStorageDeletionCompensationService;
import com.iflytek.skillhub.domain.user.UserAccount;
import com.iflytek.skillhub.infra.jpa.NamespaceJpaRepository;
import com.iflytek.skillhub.infra.jpa.PromotionRequestJpaRepository;
import com.iflytek.skillhub.infra.jpa.PromotionRevocationHistoryJpaRepository;
import com.iflytek.skillhub.infra.jpa.SkillJpaRepository;
import com.iflytek.skillhub.infra.jpa.SkillVersionJpaRepository;
import com.iflytek.skillhub.infra.jpa.UserAccountJpaRepository;
import com.iflytek.skillhub.storage.ObjectStorageService;
import java.time.Clock;
import java.util.Map;
import java.util.Set;
import java.util.UUID;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabase;
import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest;
import org.springframework.boot.test.context.TestConfiguration;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Import;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.test.context.ActiveProfiles;
import org.springframework.test.context.DynamicPropertyRegistry;
import org.springframework.test.context.DynamicPropertySource;
import org.springframework.transaction.PlatformTransactionManager;
import org.springframework.transaction.annotation.Propagation;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.transaction.support.TransactionTemplate;
import org.testcontainers.containers.PostgreSQLContainer;
import org.testcontainers.junit.jupiter.Container;
import org.testcontainers.junit.jupiter.Testcontainers;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
/** Exercises the deletion boundary with real foreign keys, commits and retained audit history. */
@DataJpaTest(showSql = false)
@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)
@ActiveProfiles("test")
@Testcontainers
@Import({PromotionRevocationService.class, PromotionRevocationHistoryJpaRepository.class,
SkillHardDeleteService.class, AuditLogService.class, PromotionDeletionPostgresTest.Config.class})
@Transactional(propagation = Propagation.NOT_SUPPORTED)
class PromotionDeletionPostgresTest {
@Container
private static final PostgreSQLContainer<?> POSTGRES = new PostgreSQLContainer<>("postgres:16-alpine");
@DynamicPropertySource
static void postgres(DynamicPropertyRegistry registry) {
registry.add("spring.datasource.url", POSTGRES::getJdbcUrl);
registry.add("spring.datasource.username", POSTGRES::getUsername);
registry.add("spring.datasource.password", POSTGRES::getPassword);
registry.add("spring.datasource.driver-class-name", () -> "org.postgresql.Driver");
registry.add("spring.jpa.database-platform", () -> "org.hibernate.dialect.PostgreSQLDialect");
registry.add("spring.flyway.enabled", () -> true);
registry.add("spring.jpa.hibernate.ddl-auto", () -> "validate");
}
@TestConfiguration
static class Config {
@Bean Clock clock() { return Clock.systemUTC(); }
@Bean ObjectMapper objectMapper() { return new ObjectMapper(); }
}
@Autowired private NamespaceJpaRepository namespaces;
@Autowired private UserAccountJpaRepository users;
@Autowired private SkillJpaRepository skills;
@Autowired private SkillVersionJpaRepository versions;
@Autowired private PromotionRequestJpaRepository promotions;
@Autowired private PromotionRevocationService revocations;
@Autowired private SkillHardDeleteService deletion;
@Autowired private PlatformTransactionManager transactions;
@Autowired private JdbcTemplate jdbc;
@MockBean private ObjectStorageService storage;
@MockBean private SkillStorageDeletionCompensationService compensation;
@MockBean private SecurityScanService scanning;
@Test
void rejectedUpdateDoesNotRemoveThePublishedPromotion() {
Graph graph = fixture(ReviewTaskStatus.REJECTED);
assertDeletionBlocked(graph.source(), graph.teamSlug(), graph.owner());
assertDeletionBlocked(graph.target(), "global", graph.owner());
assertThat(promotions.findActiveInitialBySourceSkillId(graph.source().getId())).isPresent();
assertThat(skills.existsById(graph.target().getId())).isTrue();
assertThat(jdbc.queryForObject("SELECT count(*) FROM audit_log WHERE action = 'DELETE_SKILL_HARD' AND target_id IN (?, ?)",
Integer.class, graph.source().getId(), graph.target().getId())).isZero();
}
@Test
void onlyApprovedRevocationRemovesGlobalAndAllowsSourceDeletionWithAuditRetained() {
Graph graph = fixture(ReviewTaskStatus.PENDING);
var request = revocations.submit(graph.source().getId(), graph.owner(), Map.of(), Set.of(), "Remove global listing");
assertDeletionBlocked(graph.source(), graph.teamSlug(), graph.owner());
assertDeletionBlocked(graph.target(), "global", graph.owner());
revocations.approve(request.getId(), graph.reviewer(), Set.of("SUPER_ADMIN"), "Approved", null, null);
assertThat(skills.existsById(graph.target().getId())).isFalse();
assertThat(skills.existsById(graph.source().getId())).isTrue();
assertThat(promotions.findActiveInitialBySourceSkillId(graph.source().getId())).isEmpty();
assertThat(jdbc.queryForList("SELECT target_skill_id FROM promotion_request WHERE source_skill_id = ?",
Long.class, graph.source().getId())).containsOnlyNulls();
assertThat(jdbc.queryForObject("SELECT status FROM promotion_request WHERE id = ?",
String.class, graph.updateId())).isEqualTo("REJECTED");
assertThat(jdbc.queryForObject("SELECT target_skill_id_snapshot FROM promotion_request WHERE id = ?",
Long.class, graph.initialId())).isEqualTo(graph.target().getId());
assertThat(jdbc.queryForObject("SELECT count(*) FROM skill_version WHERE skill_id = ?",
Integer.class, graph.source().getId())).isEqualTo(2);
deletion.hardDeleteSkill(graph.source(), graph.teamSlug(), graph.owner(), null, null);
assertThat(skills.existsById(graph.source().getId())).isFalse();
assertThat(jdbc.queryForObject("SELECT status FROM promotion_revocation_request WHERE id = ?",
String.class, request.getId())).isEqualTo("APPROVED");
assertThat(jdbc.queryForObject("SELECT count(*) FROM audit_log WHERE action = 'REVOKE_PROMOTION_TARGET' AND target_id = ?",
Integer.class, graph.target().getId())).isEqualTo(1);
assertThat(jdbc.queryForObject("SELECT count(*) FROM audit_log WHERE action = 'DELETE_SKILL_HARD' AND target_id = ?",
Integer.class, graph.source().getId())).isEqualTo(1);
}
private void assertDeletionBlocked(Skill skill, String namespace, String actor) {
assertThatThrownBy(() -> deletion.hardDeleteSkill(skill, namespace, actor, null, null))
.isInstanceOf(DomainBadRequestException.class).hasMessageContaining("promotion.revocation.required");
}
private Graph fixture(ReviewTaskStatus updateStatus) {
return new TransactionTemplate(transactions).execute(status -> {
String suffix = UUID.randomUUID().toString().substring(0, 8);
String owner = "delete-owner-" + suffix;
String reviewer = "delete-admin-" + suffix;
users.saveAndFlush(new UserAccount(owner, owner, null, null));
users.saveAndFlush(new UserAccount(reviewer, reviewer, null, null));
Namespace team = namespaces.saveAndFlush(new Namespace("delete-" + suffix, "Deletion team", owner));
Namespace global = namespaces.findBySlug("global").orElseGet(() -> {
Namespace created = new Namespace("global", "Global", reviewer);
created.setType(NamespaceType.GLOBAL);
return namespaces.saveAndFlush(created);
});
Skill source = skills.saveAndFlush(new Skill(team.getId(), "delete-" + suffix, owner, SkillVisibility.PUBLIC));
SkillVersion first = publishedVersion(source, "1.0.0", owner);
SkillVersion second = publishedVersion(source, "1.1.0", owner);
source.setLatestVersionId(second.getId());
skills.saveAndFlush(source);
Skill target = new Skill(global.getId(), source.getSlug(), owner, SkillVisibility.PUBLIC);
target.setSourceSkillId(source.getId());
target = skills.saveAndFlush(target);
SkillVersion targetVersion = publishedVersion(target, "1.0.0", owner);
target.setLatestVersionId(targetVersion.getId());
skills.saveAndFlush(target);
PromotionRequest initial = new PromotionRequest(source.getId(), first.getId(), global.getId(), owner);
initial.setStatus(ReviewTaskStatus.APPROVED);
initial.setTargetSkillId(target.getId());
initial.setTargetVersionId(targetVersion.getId());
initial = promotions.saveAndFlush(initial);
PromotionRequest update = new PromotionRequest(source.getId(), second.getId(), global.getId(), owner);
update.setRequestKind(PromotionRequestKind.UPDATE);
update.setStatus(updateStatus);
update.setTargetSkillId(target.getId());
update = promotions.saveAndFlush(update);
return new Graph(source, target, team.getSlug(), owner, reviewer, initial.getId(), update.getId());
});
}
private SkillVersion publishedVersion(Skill skill, String version, String owner) {
SkillVersion created = new SkillVersion(skill.getId(), version, owner);
created.setStatus(SkillVersionStatus.PUBLISHED);
return versions.saveAndFlush(created);
}
private record Graph(Skill source, Skill target, String teamSlug, String owner, String reviewer,
Long initialId, Long updateId) { }
}

View file

@ -23,6 +23,7 @@ const usePromotionSourceStateMock = vi.fn()
const submitPromotionMock = vi.fn()
const submitRevocationMock = vi.fn()
const directRevocationMock = vi.fn()
const deleteSkillMock = vi.fn()
const useRevocationHistoryMock = vi.fn()
const routeParams = { namespace: 'global', slug: 'demo-skill' }
const searchMock = vi.hoisted(() => ({ value: { returnTo: '/dashboard/skills', version: undefined as string | undefined } }))
@ -212,7 +213,7 @@ vi.mock('@/shared/hooks/use-skill-queries', () => ({
useSkillReadme: (...args: unknown[]) => useSkillReadmeMock(...args),
useSkillFile: (...args: unknown[]) => useSkillFileMock(...args),
useArchiveSkill: () => ({ mutateAsync: vi.fn(), isPending: false }),
useDeleteSkill: () => ({ mutateAsync: vi.fn(), isPending: false }),
useDeleteSkill: () => ({ mutateAsync: deleteSkillMock, isPending: false }),
useDeleteSkillVersion: () => ({ mutateAsync: vi.fn(), isPending: false }),
useRereleaseSkillVersion: () => ({ mutateAsync: vi.fn(), isPending: false }),
useUnarchiveSkill: () => ({ mutateAsync: vi.fn(), isPending: false }),
@ -296,6 +297,7 @@ describe('SkillDetailPage', () => {
usePromotionSourceStateMock.mockReturnValue({ data: undefined, isLoading: false, error: null })
searchMock.value = { returnTo: '/dashboard/skills', version: undefined }
navigateMock.mockReset()
deleteSkillMock.mockReset()
useSkillFilesMock.mockReset()
useSkillReadmeMock.mockReset()
useSkillFileMock.mockReset()
@ -334,6 +336,26 @@ describe('SkillDetailPage', () => {
useSkillFileMock.mockReturnValue({ data: null, isLoading: false, error: null })
})
it('consumes protected deletion failures at the actual button boundary and allows retry', async () => {
deleteSkillMock.mockRejectedValue(new Error('revoke global publication first'))
render(<SkillDetailPage />)
fireEvent.click(screen.getByRole('button', { name: 'skillDetail.deleteSkill' }))
fireEvent.click(within(screen.getByRole('dialog', { name: 'skillDetail.deleteSkillConfirmTitle' }))
.getByRole('button', { name: 'skillDetail.deleteSkillContinue' }))
const dialog = await screen.findByRole('dialog', { name: 'skillDetail.deleteSkillInputTitle' })
fireEvent.change(within(dialog).getByPlaceholderText('skillDetail.deleteSkillInputPlaceholder'), { target: { value: 'demo-skill' } })
for (let attempt = 1; attempt <= 2; attempt++) {
fireEvent.click(within(dialog).getByRole('button', { name: 'skillDetail.deleteSkillFinal' }))
await waitFor(() => expect(deleteSkillMock).toHaveBeenCalledTimes(attempt))
expect(within(dialog).getByDisplayValue('demo-skill')).toBeTruthy()
}
expect(deleteSkillMock).toHaveBeenLastCalledWith({ namespace: 'global', slug: 'demo-skill', ownerId: 'owner-1' })
expect(toastMocks.success).not.toHaveBeenCalled()
expect(toastMocks.error).not.toHaveBeenCalled() // The real MutationCache owns feedback, tested separately.
expect(navigateMock).not.toHaveBeenCalled()
// An uncaught async onClick rejection also makes Vitest fail this real-page run.
})
it('shows the avatar once beside the title, without an owner management section', () => {
useSkillDetailMock.mockReturnValue({ data: createSkill({ avatarUrl: '/approved-avatar.png' }), isLoading: false, isFetching: false, error: null })
const view = render(<SkillDetailPage />)

View file

@ -613,21 +613,22 @@ export function SkillDetailPage() {
}
try {
await deleteSkillMutation.mutateAsync({ namespace, slug, ownerId: skill.ownerId })
setSkillDeleted(true)
toast.success(
t('skillDetail.deleteSkillSuccessTitle'),
t('skillDetail.deleteSkillSuccessDescription', { skill: skill.displayName }),
)
setDeleteSkillInputOpen(false)
navigateAfterOverlays(() => {
navigate({ to: resolveDeletedSkillReturnTo(search.returnTo) })
})
queryClient.removeQueries({ queryKey: ['skills', namespace, slug] })
queryClient.invalidateQueries({ queryKey: ['skills', 'my'] })
} catch (error) {
toast.error(t('skillDetail.deleteSkillErrorTitle'), error instanceof Error ? error.message : '')
throw error
} catch {
// MutationCache already reports the error and handles expired sessions.
// Consume the button handler rejection without running success side effects.
return
}
setSkillDeleted(true)
toast.success(
t('skillDetail.deleteSkillSuccessTitle'),
t('skillDetail.deleteSkillSuccessDescription', { skill: skill.displayName }),
)
setDeleteSkillInputOpen(false)
navigateAfterOverlays(() => {
navigate({ to: resolveDeletedSkillReturnTo(search.returnTo) })
})
queryClient.removeQueries({ queryKey: ['skills', namespace, slug] })
queryClient.invalidateQueries({ queryKey: ['skills', 'my'] })
}
const handleDeleteVersion = async () => {

View file

@ -0,0 +1,86 @@
// @vitest-environment jsdom
import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'
import { useMutation } from '@tanstack/react-query'
import { afterEach, describe, expect, it, vi } from 'vitest'
import { App } from '@/app/providers'
import { skillLifecycleApi } from '@/api/client'
import { ApiError } from '@/shared/lib/api-error'
import { toast } from '@/shared/lib/toast'
import { useDeleteSkill } from './use-skill-queries'
vi.mock('@/app/router', () => ({ router: {} }))
vi.mock('@tanstack/react-router', () => ({ RouterProvider: () => <DeleteHarness /> }))
vi.mock('@/shared/components/toaster', () => ({ Toaster: () => null }))
vi.mock('@/shared/lib/toast', () => ({ toast: { error: vi.fn(), success: vi.fn() } }))
const blockedError = new ApiError('revoke global publication first', 409, 'revoke global publication first')
// Exercise the real application MutationCache. Like skill-detail, the caller
// reports success only; the global handler owns errors and authentication redirects.
function DeleteHarness() {
const deletion = useDeleteSkill()
const unrelated = useMutation({ mutationFn: async () => { throw blockedError } })
const deleteSkill = async () => {
try {
await deletion.mutateAsync({ namespace: 'team', slug: 'promoted', ownerId: 'owner' })
toast.success('deleted')
} catch {
// The click boundary consumes rejection; MutationCache already handled it.
}
}
return <>
<button onClick={() => { void deleteSkill() }} disabled={deletion.isPending}>Delete</button>
<button onClick={() => { void unrelated.mutateAsync().catch(() => undefined) }}>Other request</button>
</>
}
afterEach(() => {
cleanup()
vi.restoreAllMocks()
vi.clearAllMocks()
vi.unstubAllGlobals()
})
describe('skill deletion feedback ownership', () => {
it('shows one actionable error per failed deletion, including a new retry', async () => {
const request = vi.spyOn(skillLifecycleApi, 'deleteSkill').mockRejectedValue(blockedError)
render(<App />)
fireEvent.click(screen.getByRole('button', { name: 'Delete' }))
await waitFor(() => expect(toast.error).toHaveBeenCalledTimes(1))
expect(toast.error).toHaveBeenLastCalledWith(blockedError.serverMessage)
await waitFor(() => expect(screen.getByRole('button', { name: 'Delete' }).hasAttribute('disabled')).toBe(false))
fireEvent.click(screen.getByRole('button', { name: 'Delete' }))
await waitFor(() => expect(request).toHaveBeenCalledTimes(2))
await waitFor(() => expect(toast.error).toHaveBeenCalledTimes(2))
expect(toast.success).not.toHaveBeenCalled()
})
it('reports a successful deletion once without an error notification', async () => {
vi.spyOn(skillLifecycleApi, 'deleteSkill').mockResolvedValue({ skillId: 123 })
render(<App />)
fireEvent.click(screen.getByRole('button', { name: 'Delete' }))
await waitFor(() => expect(toast.success).toHaveBeenCalledTimes(1))
expect(toast.error).not.toHaveBeenCalled()
})
it('keeps the existing login redirect and one notification when the session expires', async () => {
vi.spyOn(skillLifecycleApi, 'deleteSkill').mockRejectedValue(new ApiError('session expired', 401))
render(<App />)
const location = { href: '' }
// Override only location on a delegated window; React still sees the real DOM.
const testWindow = Object.create(window)
Object.defineProperty(testWindow, 'location', { value: location })
vi.stubGlobal('window', testWindow)
fireEvent.click(screen.getByRole('button', { name: 'Delete' }))
await waitFor(() => expect(location.href).toBe('/login'))
expect(toast.error).toHaveBeenCalledTimes(1)
expect(toast.success).not.toHaveBeenCalled()
})
it('retains global error feedback for mutations without local ownership', async () => {
render(<App />)
fireEvent.click(screen.getByRole('button', { name: 'Other request' }))
await waitFor(() => expect(toast.error).toHaveBeenCalledExactlyOnceWith(blockedError.serverMessage))
})
})