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.

Sign up for GitHub

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

Jump to bottom

Refactor task preview for hidden case #35027

Merged
merged 7 commits into from
Feb 6, 2024
Merged

Refactor task preview for hidden case #35027

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8c802ed
refactor task preview for hidden case
dukenv0307 Jan 24, 2024
bba68ba
resolve conflict
dukenv0307 Jan 28, 2024
357ef0c
fix lint
dukenv0307 Jan 28, 2024
caaf8c1
fix lint
dukenv0307 Jan 29, 2024
852c38d
Merge branch 'main' into fix/34052
dukenv0307 Feb 5, 2024
069e83e
revert change
dukenv0307 Feb 5, 2024
03743f7
fix lint
dukenv0307 Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@ import {ShowContextMenuContext, showContextMenuForReport} from '@components/Show
import Text from '@components/Text';
import UserDetailsTooltip from '@components/UserDetailsTooltip';
import withCurrentUserPersonalDetails from '@components/withCurrentUserPersonalDetails';
import useLocalize from '@hooks/useLocalize';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
import * as LocalePhoneNumber from '@libs/LocalePhoneNumber';
import Navigation from '@libs/Navigation/Navigation';
import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils';
import * as ReportUtils from '@libs/ReportUtils';
Expand All @@ -31,7 +29,6 @@ const propTypes = {
function MentionUserRenderer(props) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const {translate} = useLocalize();
const defaultRendererProps = _.omit(props, ['TDefaultRenderer', 'style']);
const htmlAttributeAccountID = lodashGet(props.tnode.attributes, 'accountid');
const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
Expand Down Expand Up @@ -60,7 +57,7 @@ function MentionUserRenderer(props) {
if (!_.isEmpty(htmlAttributeAccountID)) {
const user = lodashGet(personalDetails, htmlAttributeAccountID);
accountID = parseInt(htmlAttributeAccountID, 10);
displayNameOrLogin = LocalePhoneNumber.formatPhoneNumber(lodashGet(user, 'login', '')) || lodashGet(user, 'displayName', '') || translate('common.hidden');
displayNameOrLogin = ReportUtils.getDisplayNameForParticipant(accountID);
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like we need to revert this change. I tested a case that:
We have 3 users, UserA, UserB and UserC. UserA and UserB already chatted. But they haven't chatted with UserC

  1. UserA open chat with UserB
  2. UserA create a task, assign to UserC and shared in chat with UserB
  3. Login in UserB and open chat with UserA

With this change, I'm seeing:
Screenshot 2024-02-02 at 18 17 40

But if I visit a task, and then back, it displays "Hidden"
Screenshot 2024-02-02 at 18 18 28

I think it's because of this logic

App/src/libs/ReportUtils.ts

Lines 1615 to 1621 in d5eba65

const formattedLogin = LocalePhoneNumber.formatPhoneNumber(personalDetails.login || '');
// This is to check if account is an invite/optimistically created one
// and prevent from falling back to 'Hidden', so a correct value is shown
// when searching for a new user
if (personalDetails.isOptimisticPersonalDetail === true) {
return formattedLogin;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hoangzinh Thanks for your testing, I reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are lint errors here. Could you help to recheck? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hoangzinh I fixed.

displayNameOrLogin = getMentionDisplayText(displayNameOrLogin, htmlAttributeAccountID, lodashGet(user, 'login', ''));
navigationRoute = ROUTES.PROFILE.getRoute(htmlAttributeAccountID);
} else if (!_.isEmpty(tnode.data)) {
Expand Down
10 changes: 1 addition & 9 deletions src/components/ReportActionItem/TaskPreview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import type {OnyxEntry} from 'react-native-onyx';
import Checkbox from '@components/Checkbox';
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import {usePersonalDetails} from '@components/OnyxProvider';
import PressableWithoutFeedback from '@components/Pressable/PressableWithoutFeedback';
import RenderHTML from '@components/RenderHTML';
import {showContextMenuForReport} from '@components/ShowContextMenuContext';
Expand All @@ -20,7 +19,6 @@ import useThemeStyles from '@hooks/useThemeStyles';
import ControlSelection from '@libs/ControlSelection';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import getButtonState from '@libs/getButtonState';
import * as LocalePhoneNumber from '@libs/LocalePhoneNumber';
import Navigation from '@libs/Navigation/Navigation';
import * as ReportUtils from '@libs/ReportUtils';
import * as TaskUtils from '@libs/TaskUtils';
Expand Down Expand Up @@ -84,7 +82,6 @@ function TaskPreview({
}: TaskPreviewProps) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
const {translate} = useLocalize();

// The reportAction might not contain details regarding the taskReport
Expand All @@ -95,13 +92,8 @@ function TaskPreview({
: action?.childStateNum === CONST.REPORT.STATE_NUM.APPROVED && action?.childStatusNum === CONST.REPORT.STATUS_NUM.APPROVED;
const taskTitle = Str.htmlEncode(TaskUtils.getTaskTitle(taskReportID, action?.childReportName ?? ''));
const taskAssigneeAccountID = Task.getTaskAssigneeAccountID(taskReport) ?? action?.childManagerAccountID ?? '';
const assigneeLogin = personalDetails[taskAssigneeAccountID]?.login ?? '';
const assigneeDisplayName = personalDetails[taskAssigneeAccountID]?.displayName ?? '';
const taskAssignee = assigneeDisplayName || LocalePhoneNumber.formatPhoneNumber(assigneeLogin);
const htmlForTaskPreview =
taskAssignee && taskAssigneeAccountID !== 0
? `<comment><mention-user accountid="${taskAssigneeAccountID}">@${taskAssignee}</mention-user> ${taskTitle}</comment>`
: `<comment>${taskTitle}</comment>`;
taskAssigneeAccountID !== 0 ? `<comment><mention-user accountid="${taskAssigneeAccountID}"></mention-user> ${taskTitle}</comment>` : `<comment>${taskTitle}</comment>`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any linter warning if we keep 3 lines as before instead of 1 line of code, it's hard to read code for me atm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's displayed after I run prettier.

const isDeletedParentAction = ReportUtils.isCanceledTaskReport(taskReport, action);

if (isDeletedParentAction) {
Expand Down
Loading