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

Feat(conversation)#WB-3834 draft list message preview #620

Merged
merged 9 commits into from
Mar 12, 2025
1 change: 1 addition & 0 deletions conversation/backend/src/main/resources/i18n/fr.json
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@
"progress": "Progression",
"put.trash": "Suppression du dossier",
"recipient": "Destinataire",
"recipient.list": "Liste des destinataires",
"recipient.avatar": "Avatar du destinataire",
"recipient.avatar.group": "Plusieurs destinataires",
"remove.attachment": "Supprimer la pièce jointe",
Expand Down
42 changes: 24 additions & 18 deletions conversation/frontend/src/components/MessageRecipientList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@ import clsx from 'clsx';
import { Fragment, ReactNode } from 'react';
import { Recipients } from '~/models';
import { MessageRecipientListItem } from './MessageRecipientListItem';
import { useTranslation } from 'react-i18next';

export interface RecipientListProps {
recipients: Recipients;
head: ReactNode;
head?: ReactNode;
color?: 'text-gray-800' | 'text-gray-700';
truncate?: boolean;
hasLink?: boolean;
Expand All @@ -19,25 +20,30 @@ export function MessageRecipientList({
hasLink = false,
}: RecipientListProps) {
const recipientArray = [...recipients.users, ...recipients.groups];

const { t } = useTranslation('conversation');
return (
<div className={clsx({ 'text-truncate': truncate }, color)}>
<span className="text-uppercase me-4">{head}</span>
{recipientArray.map((recipient, index) => {
const type = index < recipients.users.length ? 'user' : 'group';
const isLast = index === recipientArray.length - 1;
return (
<Fragment key={recipient.id}>
<MessageRecipientListItem
recipient={recipient}
color={color}
type={type}
hasLink={hasLink}
/>
{!isLast && ', '}
</Fragment>
);
})}
{head && <span className="text-uppercase me-4">{head}</span>}
<ul
className={'list-unstyled mb-0 d-inline'}
aria-label={t('recipient.list')}
>
{recipientArray.map((recipient, index) => {
const type = index < recipients.users.length ? 'user' : 'group';
const isLast = index === recipientArray.length - 1;
return (
<li key={recipient.id} className="d-inline">
<MessageRecipientListItem
recipient={recipient}
color={color}
type={type}
hasLink={hasLink}
/>
{!isLast && ', '}
</li>
);
})}
</ul>
</div>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ export function RecipientAvatar({ recipients }: RecipientAvatarProps) {
const { t } = useTranslation('conversation');
const { recipientCount, url } = useRecipientAvatar(recipients);

if (recipientCount > 1) {
if (recipientCount === 0) {

Choose a reason for hiding this comment

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

Tu peux refacto pour éviter la répétition et virer le else inutile :

if (recipientCount === 1 && url) {
  return <Avatar
      alt={t('recipient.avatar')}
      size="sm"
      src={url}
      variant="circle"
    />;
}

const avatars = {
  0: {
    bg: 'bg-yellow-200',
    label: t('recipient.avatar.empty'),
    icon: <IconQuestionMark className="w-16" />
  },
  1: {
    bg: 'bg-orange-200',
    label: t('recipient.avatar.group'),
    icon: <IconGroupAvatar className="w-16" />
  },
};

const avatar = avatars[Math.min(recipientCount, 1)];

return avatar ? (
  <div
    className={`${avatar.bg} avatar avatar-sm rounded-circle`}
    aria-label={avatar.label}
    role="img"
  >
    {avatar.icon}
  </div>
) : null;

return null;
} else if (recipientCount > 1) {
return (
<div
className="bg-orange-200 avatar avatar-sm rounded-circle"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,13 @@ import { MessageMetadata } from '~/models';

export interface RecipientListPreviewProps {
message: MessageMetadata;
hasPrefix?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hasPrefix?: boolean;
head?: string;

}

export function RecipientListPreview({ message }: RecipientListPreviewProps) {
export function RecipientListPreview({
message,
hasPrefix,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hasPrefix,
head,

ou }, ...otherProps

}: RecipientListPreviewProps) {
const { t } = useTranslation('conversation');
const to = message.to;
const cc = message.cc;
Expand All @@ -17,7 +21,7 @@ export function RecipientListPreview({ message }: RecipientListPreviewProps) {
};
return (
<MessageRecipientList
head={t('at')}
head={hasPrefix ? t('at') : null}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
head={hasPrefix ? t('at') : null}
head={head}

ou
{...otherProps}

recipients={recipients}
color="text-gray-800"
truncate
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ describe('Message preview header component', () => {
expect(messageHasAttachements).not.toBeInTheDocument();
});

it('should display "to" label and recipient name when is in outbox', async () => {
it('should display "to" label and recipient name when in outbox', async () => {
mocks.useParams.mockReturnValue({ folderId: 'outbox' });

render(<MessagePreview message={message} />);
Expand All @@ -119,7 +119,7 @@ describe('Message preview header component', () => {
expect(senderName).toBeInTheDocument();
});

it('should display the recipient avatar when is in outbox', async () => {
it('should display the recipient avatar when in outbox', async () => {
mocks.useParams.mockReturnValue({ folderId: 'outbox' });

const messageWithOneRecipient = mockMessagesOfInbox[1];
Expand All @@ -131,7 +131,7 @@ describe('Message preview header component', () => {
expect(recipientAvatar).toBeInTheDocument();
});

it('should display group avatar icon when more than one recipient in outbox', async () => {
it('should display group avatar icon when more than one recipient when in outbox', async () => {
mocks.useParams.mockReturnValue({ folderId: 'outbox' });
render(<MessagePreview message={message} />);

Expand All @@ -141,13 +141,43 @@ describe('Message preview header component', () => {
expect(recipientAvatar).toBeInTheDocument();
});

it('should display all recipients in after "to" label in outbox', async () => {
it('should display all recipients after "to" label when in outbox', async () => {
mocks.useParams.mockReturnValue({ folderId: 'outbox' });
render(<MessagePreview message={mockMessageOfOutbox} />);

const atElement = screen.getByText('at');
const parentContainer = atElement.closest('div');
const spanElements = parentContainer?.querySelectorAll('span');
expect(spanElements).toHaveLength(6);
screen.getByText('at');

const recipientItems = screen.getAllByRole('listitem');
expect(recipientItems).toHaveLength(5);
});

it('should display a "draft" label when in draft', async () => {
mocks.useParams.mockReturnValue({ folderId: 'draft' });
render(<MessagePreview message={mockMessageOfOutbox} />);
screen.getByText('draft');
});

it('should display all recipients without "to" label when in draft', async () => {
mocks.useParams.mockReturnValue({ folderId: 'draft' });
render(<MessagePreview message={mockMessageOfOutbox} />);

const atElement = screen.queryByText('at');
expect(atElement).toBeNull();

const recipientItems = screen.queryAllByRole('listitem');
expect(recipientItems).toHaveLength(5);
});

it.only('should not display any recipients if there are none when in draft', async () => {
mocks.useParams.mockReturnValue({ folderId: 'draft' });
const message = { ...mockMessageOfOutbox };
message.to = { users: [], groups: [] };
message.cc = { users: [], groups: [] };
message.cci = { users: [], groups: [] };

render(<MessagePreview message={message} />);

const recipientItems = screen.queryAllByRole('listitem');
expect(recipientItems).toHaveLength(0);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,27 @@ export function MessagePreview({ message }: MessagePreviewProps) {
<IconUndo className="gray-800" title="message-response" />
)}

{'outbox' === folderId ? (
{folderId && ['outbox', 'draft'].includes(folderId) ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Dans l'ancienne messagerie, les noms des dossiers étaient en majuscule.
La comparaison ici ne tient pas compte de la casse, on risque donc d'avoir des effets de bord en ouvrant une URL à l'ancien format.
=> Il faut que le paramètre folderId soit "normalisé", et c'est justement ce que fait le hook useSelectedFolder, à utiliser à la place de useParams

Copy link
Author

Choose a reason for hiding this comment

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

@Romu-C ce hook sert donc à ça !

Copy link
Author

@damienromito damienromito Mar 11, 2025

Choose a reason for hiding this comment

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

donc on remplace useSelectedFolder partout ? histoire d'être consistant

Copy link
Author

@damienromito damienromito Mar 11, 2025

Choose a reason for hiding this comment

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

je vois qu'il y a aussi une constante SYSTEM_FOLDER_ID . Ça serait mieux de l'utiliser non ?

export const SYSTEM_FOLDER_ID = { INBOX: 'inbox', OUTBOX: 'outbox', DRAFT: 'draft', TRASH: 'trash', }

=> folderId === SYSTEM_FOLDER_ID.DRAFT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Il vaudrait mieux, oui (x2). A discuter plus en détails si le moindre doute persiste.

<RecipientAvatar recipients={message.to} />
) : (
<SenderAvatar authorId={message.from.id} />
)}

<div className="d-flex flex-fill flex-column overflow-hidden">
<div className="d-flex flex-fill justify-content-between overflow-hidden">
<div className="d-flex flex-fill justify-content-between overflow-hidden gap-4">
{folderId === 'draft' && (
<strong className="text-danger">{t('draft')}</strong>
)}
<div className="text-truncate flex-fill">
{'outbox' === folderId ? (
<RecipientListPreview message={message} />
) : (
senderDisplayName
{folderId === 'draft' && <RecipientListPreview message={message} />}
{folderId === 'outbox' && (
<RecipientListPreview message={message} hasPrefix />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<RecipientListPreview message={message} hasPrefix />
<RecipientListPreview message={message} head={t('at')} />

)}
{folderId && ['inbox', 'trash'].includes(folderId) && (
<>{senderDisplayName}</>
)}
</div>

<div className="fw-bold text-nowrap fs-12 gray-800">
{fromNow(message.date)}
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ describe('Message recipient list', () => {
});

it('should display recipient list for To but not Cc and Cci', async () => {
mockFullMessage.cc = { users: [], groups: [] };
render(<MessageHeader message={mockFullMessage} />);
const message = { ...mockFullMessage };
message.cc = { users: [], groups: [] };
render(<MessageHeader message={message} />);

const toLabel = screen.queryByText('at');
expect(toLabel).toBeInTheDocument();
Expand Down
5 changes: 3 additions & 2 deletions conversation/frontend/src/hooks/useRecipientAvatar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ import { Recipients } from '~/models';
export function useRecipientAvatar(recipients: Recipients) {
const { getAvatarURL } = useDirectory();
const recipientCount = recipients.users.length + recipients.groups.length;

if (recipientCount > 1) {
if (recipientCount === 0) {
return { recipientCount, url: null };
} else if (recipientCount > 1) {
return { recipientCount, url: null };
} else {
const firstRecipient = recipients.users[0] || recipients.groups[0];
Expand Down
41 changes: 17 additions & 24 deletions conversation/frontend/src/mocks/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { Folder, Message, MessageMetadata } from '~/models';
import { Folder, Message, MessageMetadata, User } from '~/models';

const mockCurrentUserPreview: User = {
displayName: 'LOISON Stéphane',
id: 'b92e3d37-16b0-4ed9-b4c3-992091687132',
profile: 'Teacher',
};

export const mockFolderTree: Array<Folder> = [
{
Expand Down Expand Up @@ -42,13 +48,13 @@ export const mockMessagesOfInbox: MessageMetadata[] = [
},
state: 'SENT',
date: 1503571892555,
unread: true,
unread: false,
response: false,
trashed: false,
forwarded: false,
hasAttachment: false,
to: {
users: [],
users: [mockCurrentUserPreview],
groups: [
{
id: '465',
Expand All @@ -71,6 +77,11 @@ export const mockMessagesOfInbox: MessageMetadata[] = [
type: 'ProfileGroup',
subType: 'StructureGroup',
},
],
},
cc: {
users: [],
groups: [
{
id: '466',
displayName: 'Personnels du groupe scolaire.',
Expand All @@ -80,10 +91,6 @@ export const mockMessagesOfInbox: MessageMetadata[] = [
},
],
},
cc: {
users: [],
groups: [],
},
cci: {
users: [],
groups: [],
Expand Down Expand Up @@ -130,11 +137,7 @@ export const mockMessagesOfInbox: MessageMetadata[] = [
export const mockMessageOfOutbox: MessageMetadata = {
id: 'f43d3783',
subject: 'Prêt des manuels scolaires',
from: {
displayName: 'LOISON Stéphane',
id: 'b92e3d37-16b0-4ed9-b4c3-992091687132',
profile: 'Teacher',
},
from: mockCurrentUserPreview,
state: 'SENT',
date: 1503571892555,
unread: true,
Expand Down Expand Up @@ -172,13 +175,7 @@ export const mockMessageOfOutbox: MessageMetadata = {
groups: [],
},
cci: {
users: [
{
displayName: 'LOISON Stéphane',
id: 'b92e3d37-16b0-4ed9-b4c3-992091687132',
profile: 'Teacher',
},
],
users: [mockCurrentUserPreview],
groups: [
{
id: '467',
Expand Down Expand Up @@ -225,11 +222,7 @@ export const mockFullMessage: Message = {
],
to: {
users: [
{
displayName: 'LOISON Stéphane',
id: 'b92e3d37-16b0-4ed9-b4c3-992091687132',
profile: 'Teacher',
},
mockCurrentUserPreview,
{
id: '91c22b66',
displayName: 'ISABELLE POLONIO (prof arts plastiques)',
Expand Down