From 6bf62e054a1ab0451f9e392c149e88d1db4f35bd Mon Sep 17 00:00:00 2001 From: Harmit Goswami Date: Thu, 10 Oct 2024 16:56:15 -0400 Subject: [PATCH] Addressed fourth round of review comments --- pontoon/base/models/comment.py | 2 +- pontoon/base/models/user.py | 4 +- pontoon/base/tests/models/test_user.py | 12 +-- pontoon/base/views.py | 2 +- translate/src/api/comment.ts | 2 +- translate/src/api/translation.ts | 2 +- translate/src/api/user.ts | 1 + translate/src/hooks/useUserStatus.test.js | 88 +++++++++++++++++++ .../{useLocaleRole.ts => useUserStatus.ts} | 16 ++-- .../comments/components/AddComment.tsx | 4 +- .../modules/comments/components/Comment.tsx | 1 - .../comments/components/CommentsList.tsx | 2 - .../history/components/HistoryTranslation.tsx | 1 - .../modules/user/components/UserAvatar.css | 22 ++--- .../modules/user/components/UserAvatar.tsx | 6 +- 15 files changed, 124 insertions(+), 41 deletions(-) create mode 100644 translate/src/hooks/useUserStatus.test.js rename translate/src/hooks/{useLocaleRole.ts => useUserStatus.ts} (64%) diff --git a/pontoon/base/models/comment.py b/pontoon/base/models/comment.py index 9b9422c884..7ecb88f29c 100644 --- a/pontoon/base/models/comment.py +++ b/pontoon/base/models/comment.py @@ -30,7 +30,7 @@ def serialize(self): return { "author": self.author.name_or_email, "username": self.author.username, - "user_status": self.author.locale_status(self.locale), + "user_status": self.author.status(self.locale), "user_gravatar_url_small": self.author.gravatar_url(88), "created_at": self.timestamp.strftime("%b %d, %Y %H:%M"), "date_iso": self.timestamp.isoformat(), diff --git a/pontoon/base/models/user.py b/pontoon/base/models/user.py index 1ba4f1f721..e9e6a1a81c 100644 --- a/pontoon/base/models/user.py +++ b/pontoon/base/models/user.py @@ -194,7 +194,7 @@ def user_locale_role(self, locale): return "Contributor" -def user_locale_status(self, locale): +def user_status(self, locale): if self.username == "Imported": return ("", "") if self.is_superuser: @@ -439,7 +439,7 @@ def latest_action(self): User.add_to_class("translated_projects", user_translated_projects) User.add_to_class("role", user_role) User.add_to_class("locale_role", user_locale_role) -User.add_to_class("locale_status", user_locale_status) +User.add_to_class("status", user_status) User.add_to_class("contributed_translations", contributed_translations) User.add_to_class("top_contributed_locale", top_contributed_locale) User.add_to_class("can_translate", can_translate) diff --git a/pontoon/base/tests/models/test_user.py b/pontoon/base/tests/models/test_user.py index 5b7b3e11d9..e4d4ac5520 100644 --- a/pontoon/base/tests/models/test_user.py +++ b/pontoon/base/tests/models/test_user.py @@ -64,22 +64,22 @@ def test_user_locale_role(user_a, user_b, user_c, locale_a): @pytest.mark.django_db -def test_user_locale_status(user_a, user_b, user_c, locale_a): +def test_user_status(user_a, user_b, user_c, locale_a): # New User - assert user_a.locale_status(locale_a)[1] == "New User" + assert user_a.status(locale_a)[1] == "New User" # Fake user object imported = User(username="Imported") - assert imported.locale_status(locale_a)[1] == "" + assert imported.status(locale_a)[1] == "" # Admin user_a.is_superuser = True - assert user_a.locale_status(locale_a)[1] == "Admin" + assert user_a.status(locale_a)[1] == "Admin" # Manager locale_a.managers_group.user_set.add(user_b) - assert user_b.locale_status(locale_a)[1] == "Manager" + assert user_b.status(locale_a)[1] == "Manager" # Translator locale_a.translators_group.user_set.add(user_c) - assert user_c.locale_status(locale_a)[1] == "Translator" + assert user_c.status(locale_a)[1] == "Translator" diff --git a/pontoon/base/views.py b/pontoon/base/views.py index 3746f1d7a6..313ad2b707 100755 --- a/pontoon/base/views.py +++ b/pontoon/base/views.py @@ -420,7 +420,7 @@ def get_translation_history(request): "uid": u.id, "username": u.username, "user_gravatar_url_small": u.gravatar_url(88), - "user_status": u.locale_status(locale), + "user_status": u.status(locale), "date": t.date, "approved_user": User.display_name_or_blank(t.approved_user), "approved_date": t.approved_date, diff --git a/translate/src/api/comment.ts b/translate/src/api/comment.ts index a6e0a27718..8d6f8c006b 100644 --- a/translate/src/api/comment.ts +++ b/translate/src/api/comment.ts @@ -9,7 +9,7 @@ import { keysToCamelCase } from './utils/keysToCamelCase'; export type TranslationComment = { readonly author: string; readonly username: string; - readonly userStatus: string; + readonly userStatus: string[]; readonly userGravatarUrlSmall: string; readonly createdAt: string; readonly dateIso: string; diff --git a/translate/src/api/translation.ts b/translate/src/api/translation.ts index 3f53e1819f..9dc56de83c 100644 --- a/translate/src/api/translation.ts +++ b/translate/src/api/translation.ts @@ -44,7 +44,7 @@ export type HistoryTranslation = { readonly user: string; readonly username: string; readonly userGravatarUrlSmall: string; - readonly userStatus: string; + readonly userStatus: string[]; readonly comments: Array; }; diff --git a/translate/src/api/user.ts b/translate/src/api/user.ts index 15198f0427..98a25f1b0e 100644 --- a/translate/src/api/user.ts +++ b/translate/src/api/user.ts @@ -30,6 +30,7 @@ export type ApiUserData = { name_or_email?: string; email?: string; username?: string; + date_joined?: string; manager_for_locales?: string[]; translator_for_locales?: string[]; translator_for_projects?: Record; diff --git a/translate/src/hooks/useUserStatus.test.js b/translate/src/hooks/useUserStatus.test.js new file mode 100644 index 0000000000..6381b1b3a4 --- /dev/null +++ b/translate/src/hooks/useUserStatus.test.js @@ -0,0 +1,88 @@ +import React from 'react'; +import sinon from 'sinon'; + +import { USER } from '~/modules/user'; +import * as Hooks from '~/hooks'; + +import { useUserStatus } from './useUserStatus'; + +beforeAll(() => { + sinon.stub(Hooks, 'useAppSelector'); + sinon.stub(React, 'useContext').returns({ code: 'mylocale' }); +}); +afterAll(() => { + Hooks.useAppSelector.restore(); + React.useContext.restore(); +}); + +const fakeSelector = (user) => (sel) => + sel({ + [USER]: user, + }); + +describe('useUserStatus', () => { + it('returns empty parameters for non-authenticated users', () => { + Hooks.useAppSelector.callsFake(fakeSelector({ isAuthenticated: false })), + expect(useUserStatus()).toStrictEqual(['', '']); + }); + + it('returns [ADMIN, Admin] if user has admin permissions', () => { + Hooks.useAppSelector.callsFake( + fakeSelector({ + isAuthenticated: true, + isAdmin: true, + managerForLocales: ['mylocale'], + translatorForLocales: [], + }), + ); + expect(useUserStatus()).toStrictEqual(['ADMIN', 'Admin']); + }); + + it('returns [MNGR, Manager] if user is a manager of the locale', () => { + Hooks.useAppSelector.callsFake( + fakeSelector({ + isAuthenticated: true, + managerForLocales: ['mylocale'], + translatorForLocales: [], + }), + ); + expect(useUserStatus()).toStrictEqual(['MNGR', 'Manager']); + }); + + it('returns [TRNSL, Translator] if user is a translator for the locale', () => { + Hooks.useAppSelector.callsFake( + fakeSelector({ + isAuthenticated: true, + managerForLocales: [], + translatorForLocales: ['mylocale'], + }), + ); + expect(useUserStatus()).toStrictEqual(['TRNSL', 'Translator']); + }); + + it('returns [NEW, New User] if user created their account within the last 3 months', () => { + const dateJoined = new Date(); + dateJoined.setMonth(dateJoined.getMonth() - 2); + Hooks.useAppSelector.callsFake( + fakeSelector({ + isAuthenticated: true, + managerForLocales: [], + translatorForLocales: [], + dateJoined: dateJoined, + }), + ); + expect(useUserStatus()).toStrictEqual(['NEW', 'New User']); + + // Set join date to be 6 months ago (no longer a new user) + dateJoined.setMonth(dateJoined.getMonth() - 6); + Hooks.useAppSelector.callsFake( + fakeSelector({ + isAuthenticated: true, + managerForLocales: [], + translatorForLocales: [], + dateJoined: dateJoined, + }), + ); + expect(useUserStatus()).toStrictEqual(['', '']); + }); +}); diff --git a/translate/src/hooks/useLocaleRole.ts b/translate/src/hooks/useUserStatus.ts similarity index 64% rename from translate/src/hooks/useLocaleRole.ts rename to translate/src/hooks/useUserStatus.ts index 9a5cd76572..05589cd795 100644 --- a/translate/src/hooks/useLocaleRole.ts +++ b/translate/src/hooks/useUserStatus.ts @@ -7,7 +7,7 @@ import { useAppSelector } from '~/hooks'; /** * Return the user's status within the given locale, to display on the user banner */ -export function useLocaleRole(): Array { +export function useUserStatus(): Array { const { code } = useContext(Locale); const { isAuthenticated, @@ -25,19 +25,19 @@ export function useLocaleRole(): Array { return ['ADMIN', 'Admin']; } - if (Object.hasOwnProperty.call(managerForLocales, code)) { - return ['MNGR', 'Manger']; + if (managerForLocales.includes(code)) { + return ['MNGR', 'Manager']; } - if (Object.hasOwnProperty.call(translatorForLocales, code)) { + if (translatorForLocales.includes(code)) { return ['TRNSL', 'Translator']; } const dateJoinedObj = new Date(dateJoined); - let newUserDate = dateJoinedObj; - newUserDate.setMonth(dateJoinedObj.getMonth() - 3); - if (dateJoinedObj < newUserDate) { - return ['NEW USER', 'New User']; + let threeMonthsAgo = new Date(); + threeMonthsAgo.setMonth(threeMonthsAgo.getMonth() - 3); + if (dateJoinedObj > threeMonthsAgo) { + return ['NEW', 'New User']; } return ['', '']; diff --git a/translate/src/modules/comments/components/AddComment.tsx b/translate/src/modules/comments/components/AddComment.tsx index 708aab07d9..d2f353e250 100644 --- a/translate/src/modules/comments/components/AddComment.tsx +++ b/translate/src/modules/comments/components/AddComment.tsx @@ -30,7 +30,7 @@ import type { MentionUser } from '~/api/user'; import { MentionUsers } from '~/context/MentionUsers'; import type { UserState } from '~/modules/user'; import { UserAvatar } from '~/modules/user'; -import { useLocaleRole } from '~/hooks/useLocaleRole'; +import { useUserStatus } from '~/hooks/useUserStatus'; import './AddComment.css'; import { MentionList } from './MentionList'; @@ -87,7 +87,7 @@ export function AddComment({ const [mentionIndex, setMentionIndex] = useState(0); const [mentionSearch, setMentionSearch] = useState(''); const [requireUsers, setRequireUsers] = useState(false); - const role = useLocaleRole(); + const role = useUserStatus(); const { initMentions, mentionUsers } = useContext(MentionUsers); const [slateKey, resetValue] = useReducer((key) => key + 1, 0); diff --git a/translate/src/modules/comments/components/Comment.tsx b/translate/src/modules/comments/components/Comment.tsx index b7dbf1e068..ad29e8a584 100644 --- a/translate/src/modules/comments/components/Comment.tsx +++ b/translate/src/modules/comments/components/Comment.tsx @@ -9,7 +9,6 @@ import type { TranslationComment } from '~/api/comment'; import { UserAvatar } from '~/modules/user'; import './Comment.css'; -import '../../user/components/UserAvatar.css'; type Props = { comment: TranslationComment; diff --git a/translate/src/modules/comments/components/CommentsList.tsx b/translate/src/modules/comments/components/CommentsList.tsx index 0a6e9f831a..ff75b22c48 100644 --- a/translate/src/modules/comments/components/CommentsList.tsx +++ b/translate/src/modules/comments/components/CommentsList.tsx @@ -7,7 +7,6 @@ import { useAddCommentAndRefresh } from '../hooks'; import { AddComment } from './AddComment'; import { Comment } from './Comment'; import './CommentsList.css'; -import { useLocaleRole } from '~/hooks/useLocaleRole'; type Props = { translation: HistoryTranslation; @@ -19,7 +18,6 @@ export function CommentsList({ user, }: Props): React.ReactElement<'div'> { const onAddComment = useAddCommentAndRefresh(translation); - const role = useLocaleRole(); return (
diff --git a/translate/src/modules/history/components/HistoryTranslation.tsx b/translate/src/modules/history/components/HistoryTranslation.tsx index 44108df421..8655bf7891 100644 --- a/translate/src/modules/history/components/HistoryTranslation.tsx +++ b/translate/src/modules/history/components/HistoryTranslation.tsx @@ -14,7 +14,6 @@ import { withActionsDisabled } from '~/utils'; import { useTranslator } from '~/hooks/useTranslator'; import './HistoryTranslation.css'; -import '../../user/components/UserAvatar.css'; type Props = { entity: Entity; diff --git a/translate/src/modules/user/components/UserAvatar.css b/translate/src/modules/user/components/UserAvatar.css index 8c1837c425..e7b1066edf 100644 --- a/translate/src/modules/user/components/UserAvatar.css +++ b/translate/src/modules/user/components/UserAvatar.css @@ -4,6 +4,15 @@ top: 3px; } +.history .translation .user-avatar img { + border-radius: 6px; + border: 2px solid var(--icon-border-1); +} + +.history .translation:hover .user-avatar img { + border-color: var(--translation-border); +} + .user-avatar .avatar-container { position: relative; display: inline-block; @@ -44,16 +53,3 @@ .user-avatar .avatar-container .user-status-banner.new-user { color: var(--status-fuzzy); } - -.history .translation:hover .user-status-banner { - border-color: var(--translation-border); -} - -.history .translation .user-avatar img { - border-radius: 6px; - border: 2px solid var(--icon-border-1); -} - -.history .translation:hover .user-avatar img { - border-color: var(--translation-border); -} diff --git a/translate/src/modules/user/components/UserAvatar.tsx b/translate/src/modules/user/components/UserAvatar.tsx index 95dc29d66b..9f2682f388 100644 --- a/translate/src/modules/user/components/UserAvatar.tsx +++ b/translate/src/modules/user/components/UserAvatar.tsx @@ -1,11 +1,13 @@ import React from 'react'; import { Localized } from '@fluent/react'; +import './UserAvatar.css'; + type Props = { username: string; title?: string; imageUrl: string; - userStatus: string | string[]; + userStatus: string[]; }; export function UserAvatar(props: Props): React.ReactElement<'div'> { @@ -25,7 +27,7 @@ export function UserAvatar(props: Props): React.ReactElement<'div'> { User Profile - {userStatus && ( + {role && (