fix: improve mailbox role management by ensuring roles are cleared from all mailboxes when reassigning
This commit is contained in:
@@ -490,21 +490,34 @@ export function FolderSettings() {
|
|||||||
|
|
||||||
{/* Standard Folder Roles — advanced section */}
|
{/* Standard Folder Roles — advanced section */}
|
||||||
<SettingsSection title={t('standard_roles')} description={t('standard_roles_description')}>
|
<SettingsSection title={t('standard_roles')} description={t('standard_roles_description')}>
|
||||||
{STANDARD_ROLES.map((role) => (
|
{STANDARD_ROLES.map((role) => {
|
||||||
<SettingItem key={role} label={t(`role_${role}`)}>
|
// Disambiguate duplicate folder names by appending parent path
|
||||||
<Select
|
const nameCounts = new Map<string, number>();
|
||||||
value={getRoleMailboxId(role)}
|
ownMailboxes.forEach(mb => nameCounts.set(mb.name, (nameCounts.get(mb.name) || 0) + 1));
|
||||||
onChange={(value) => handleRoleChange(role, value)}
|
const getParentPath = (mb: { parentId?: string; name: string }) => {
|
||||||
options={[
|
if (!mb.parentId) return '';
|
||||||
{ value: '', label: t('role_none') },
|
const parent = ownMailboxes.find(p => p.id === mb.parentId);
|
||||||
...ownMailboxes.map(mb => ({
|
return parent ? `${parent.name}/` : '';
|
||||||
value: mb.id,
|
};
|
||||||
label: mb.name,
|
|
||||||
})),
|
return (
|
||||||
]}
|
<SettingItem key={role} label={t(`role_${role}`)}>
|
||||||
/>
|
<Select
|
||||||
</SettingItem>
|
value={getRoleMailboxId(role)}
|
||||||
))}
|
onChange={(value) => handleRoleChange(role, value)}
|
||||||
|
options={[
|
||||||
|
{ value: '', label: t('role_none') },
|
||||||
|
...ownMailboxes.map(mb => ({
|
||||||
|
value: mb.id,
|
||||||
|
label: (nameCounts.get(mb.name) || 0) > 1
|
||||||
|
? `${getParentPath(mb)}${mb.name} (${mb.id.slice(-6)})`
|
||||||
|
: mb.name,
|
||||||
|
})),
|
||||||
|
]}
|
||||||
|
/>
|
||||||
|
</SettingItem>
|
||||||
|
);
|
||||||
|
})}
|
||||||
</SettingsSection>
|
</SettingsSection>
|
||||||
</div>
|
</div>
|
||||||
);
|
);
|
||||||
|
|||||||
+10
-6
@@ -97,17 +97,19 @@ const ROLE_PRIORITY: Record<string, number> = {
|
|||||||
|
|
||||||
// Deduplicate mailboxes (e.g., "Sent" vs "Sent Mail")
|
// Deduplicate mailboxes (e.g., "Sent" vs "Sent Mail")
|
||||||
function deduplicateMailboxes(mailboxes: Mailbox[]): Mailbox[] {
|
function deduplicateMailboxes(mailboxes: Mailbox[]): Mailbox[] {
|
||||||
const roleMap = new Map<string, Mailbox>();
|
|
||||||
const result: Mailbox[] = [];
|
const result: Mailbox[] = [];
|
||||||
|
|
||||||
// First pass: collect mailboxes with roles
|
// Group role mailboxes by account so deduplication is scoped per-account
|
||||||
|
const rolesByAccount = new Map<string, Mailbox[]>();
|
||||||
mailboxes.forEach(mb => {
|
mailboxes.forEach(mb => {
|
||||||
if (mb.role) {
|
if (mb.role) {
|
||||||
roleMap.set(mb.role, mb);
|
const key = mb.accountId || '';
|
||||||
|
if (!rolesByAccount.has(key)) rolesByAccount.set(key, []);
|
||||||
|
rolesByAccount.get(key)!.push(mb);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
// Second pass: filter out duplicates
|
// Filter out duplicates scoped to the same account
|
||||||
mailboxes.forEach(mb => {
|
mailboxes.forEach(mb => {
|
||||||
// If this mailbox has a role, always keep it
|
// If this mailbox has a role, always keep it
|
||||||
if (mb.role) {
|
if (mb.role) {
|
||||||
@@ -115,9 +117,11 @@ function deduplicateMailboxes(mailboxes: Mailbox[]): Mailbox[] {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check if this is a duplicate of a role-based mailbox
|
// Check if this is a duplicate of a role-based mailbox in the SAME account
|
||||||
|
const accountKey = mb.accountId || '';
|
||||||
|
const accountRoles = rolesByAccount.get(accountKey) || [];
|
||||||
const lowerName = mb.name.toLowerCase();
|
const lowerName = mb.name.toLowerCase();
|
||||||
const isDuplicate = Array.from(roleMap.values()).some(roleMb => {
|
const isDuplicate = accountRoles.some(roleMb => {
|
||||||
const roleLowerName = roleMb.name.toLowerCase();
|
const roleLowerName = roleMb.name.toLowerCase();
|
||||||
// Check for common duplicates: "Sent Mail" vs "Sent", etc.
|
// Check for common duplicates: "Sent Mail" vs "Sent", etc.
|
||||||
return lowerName.includes(roleLowerName) || roleLowerName.includes(lowerName);
|
return lowerName.includes(roleLowerName) || roleLowerName.includes(lowerName);
|
||||||
|
|||||||
@@ -226,6 +226,34 @@ describe('email-store folder management', () => {
|
|||||||
expect(client.updateMailbox).toHaveBeenCalledWith('trash-1', { role: 'trash' });
|
expect(client.updateMailbox).toHaveBeenCalledWith('trash-1', { role: 'trash' });
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should clear role from ALL mailboxes with that role when reassigning', async () => {
|
||||||
|
// Simulate server anomaly: two mailboxes with role "trash"
|
||||||
|
const extraTrash = makeMailbox({ id: 'trash-2', name: 'Deleted Items', role: 'trash' });
|
||||||
|
useEmailStore.setState({
|
||||||
|
mailboxes: [inbox, sent, trash, custom, extraTrash],
|
||||||
|
});
|
||||||
|
|
||||||
|
const newMailboxes = [inbox, sent, custom,
|
||||||
|
makeMailbox({ id: 'trash-1', name: 'Trash', role: undefined }),
|
||||||
|
makeMailbox({ id: 'trash-2', name: 'Deleted Items', role: undefined }),
|
||||||
|
];
|
||||||
|
// custom-1 gets the trash role
|
||||||
|
newMailboxes[2] = { ...newMailboxes[2], role: 'trash' };
|
||||||
|
|
||||||
|
const client = makeMockClient({
|
||||||
|
getAllMailboxes: vi.fn().mockResolvedValue(newMailboxes),
|
||||||
|
});
|
||||||
|
|
||||||
|
await useEmailStore.getState().setMailboxRole(client, 'custom-1', 'trash');
|
||||||
|
|
||||||
|
// Should clear trash role from BOTH trash-1 and trash-2
|
||||||
|
expect(client.updateMailbox).toHaveBeenCalledWith('trash-1', { role: null });
|
||||||
|
expect(client.updateMailbox).toHaveBeenCalledWith('trash-2', { role: null });
|
||||||
|
// Then set trash role on custom-1
|
||||||
|
expect(client.updateMailbox).toHaveBeenCalledWith('custom-1', { role: 'trash' });
|
||||||
|
expect(client.updateMailbox).toHaveBeenCalledTimes(3);
|
||||||
|
});
|
||||||
|
|
||||||
it('should set error on failure', async () => {
|
it('should set error on failure', async () => {
|
||||||
const client = makeMockClient({
|
const client = makeMockClient({
|
||||||
updateMailbox: vi.fn().mockRejectedValue(new Error('Role update failed')),
|
updateMailbox: vi.fn().mockRejectedValue(new Error('Role update failed')),
|
||||||
|
|||||||
@@ -1298,11 +1298,11 @@ export const useEmailStore = create<EmailStore>((set, get) => ({
|
|||||||
|
|
||||||
setMailboxRole: async (client, mailboxId, role) => {
|
setMailboxRole: async (client, mailboxId, role) => {
|
||||||
try {
|
try {
|
||||||
// If assigning a role, first clear that role from any other mailbox
|
// If assigning a role, first clear that role from ALL other mailboxes that have it
|
||||||
if (role) {
|
if (role) {
|
||||||
const existingMailbox = get().mailboxes.find(mb => mb.role === role && !mb.isShared);
|
const existingMailboxes = get().mailboxes.filter(mb => mb.role === role && !mb.isShared && mb.id !== mailboxId);
|
||||||
if (existingMailbox && existingMailbox.id !== mailboxId) {
|
for (const existing of existingMailboxes) {
|
||||||
await client.updateMailbox(existingMailbox.id, { role: null });
|
await client.updateMailbox(existing.id, { role: null });
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
await client.updateMailbox(mailboxId, { role });
|
await client.updateMailbox(mailboxId, { role });
|
||||||
|
|||||||
Reference in New Issue
Block a user