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

Remove Mandatory Description field on edit task #19749

Merged
merged 5 commits into from
Jun 2, 2023
Merged

Remove Mandatory Description field on edit task #19749

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ff5f9c1
Remove Mandatory Description field on edit task
dhairyasenjaliya May 29, 2023
a19a8de
Removed Un-used import
dhairyasenjaliya May 29, 2023
dddcbf9
Label correction
dhairyasenjaliya May 29, 2023
298c57f
added bit explanation for optional validate
dhairyasenjaliya Jun 2, 2023
9842146
Update src/pages/tasks/NewTaskDescriptionPage.js
dhairyasenjaliya Jun 2, 2023
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
4 changes: 2 additions & 2 deletions src/libs/actions/Task.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ function editTaskAndNavigate(report, ownerEmail, title, description, assignee) {
key: `${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`,
value: {
reportName,
description: (description || report.description).trim(),
description: description.trim(),
Copy link
Contributor

Choose a reason for hiding this comment

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

@dhairyasenjaliya @fedirjh This change makes a regression, that is after editing the title of task, description is empty briefly, see more detail here https://expensify.slack.com/archives/C049HHMV9SM/p1686020401378739

managerEmail: assignee || report.managerEmail,
},
},
Expand Down Expand Up @@ -380,7 +380,7 @@ function editTaskAndNavigate(report, ownerEmail, title, description, assignee) {
{
taskReportID: report.reportID,
title: reportName,
description: (description || report.description).trim(),
description: description.trim(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes description is updated to empty when you edit the title of the task

Copy link
Contributor

Choose a reason for hiding this comment

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

With this change in this PR, I think we should call editTaskAndNavigate when update title of task in here with description is the current description of the report instead empty description

App/src/pages/tasks/TaskTitlePage.js

Line 62 in 82ce35f

TaskUtils.editTaskAndNavigate(props.task.report, props.session.email, values.title, '', '');

Copy link
Contributor

Choose a reason for hiding this comment

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

@dukenv0307 One simple fix for the issue is to pass description from props.task as below :

            TaskUtils.editTaskAndNavigate(
                props.task.report,
                props.session.email,
                values.title,
                props.task.description,
                props.task.managerEmail
            );

Copy link
Contributor Author

@dhairyasenjaliya dhairyasenjaliya Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nope. this will likely cause another regression since we are using the same edit for changing the assignee
what I suggest is to add a new parameter isDescriptionPage for the description something like the below and add true on the description page since we are allowing empty value as well

function editTaskAndNavigate(report, ownerEmail, title, description, assignee, isDescriptionPage = false) {

            value: {
                reportName,
                description: (isDescriptionPage ? description : report.description).trim(),
                managerEmail: assignee || report.managerEmail,
            },

Copy link
Contributor

@fedirjh fedirjh Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nope. this will likely cause another regression since we are using the same edit for changing the assignee

@dhairyasenjaliya Hmmm how will that cause regression ? If we update other instances , it should be fine , we have access to props.task on all tasks forms

Screenshot 2023-06-09 at 6 25 47 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

@dukenv0307 One simple fix for the issue is to pass description from props.task as below

@fedirjh yes, I recommended it here #19749 (comment)

nope. this will likely cause another regression since we are using the same edit for changing the assignee

@dhairyasenjaliya I don't think we need to add a new parameter for this function. I think we only need to add the correct description when we edit anything of task that doesn't description field.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fedirjh So if we create a PR to fix this regression in here, can you help me to revert the change in my PR here #19990

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah agree but let's say we have an empty description on edit then it will rely on props.task. description which has an old value then it will set the old value rather than empty, so what I think we need to make a way for the description field that it checks if we are coming from the description page then only we truly set those new value it maybe old, empty or new text

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's wait till the GH ticket is created

assignee: assignee || report.assignee,
editedTaskReportActionID: editTaskReportAction.reportActionID,
assigneeChatReportActionID: optimisticAssigneeAddComment ? optimisticAssigneeAddComment.reportAction.reportActionID : 0,
Expand Down
3 changes: 2 additions & 1 deletion src/pages/tasks/NewTaskDescriptionPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ const NewTaskDescriptionPage = (props) => {
* @returns {Object}
*/
function validate() {
// This field is optional and can be left blank, so we do not need to validate that it has a value.
dhairyasenjaliya marked this conversation as resolved.
Show resolved Hide resolved
return {};
}

Expand Down Expand Up @@ -86,7 +87,7 @@ const NewTaskDescriptionPage = (props) => {
<TextInput
defaultValue={props.task.description}
inputID="taskDescription"
label={props.translate('newTaskPage.description')}
label={props.translate('newTaskPage.descriptionOptional')}
ref={(el) => (inputRef.current = el)}
/>
</View>
Expand Down
21 changes: 2 additions & 19 deletions src/pages/tasks/TaskDescriptionPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import _ from 'underscore';
import React, {useCallback, useRef} from 'react';
import PropTypes from 'prop-types';
import {View} from 'react-native';
Expand Down Expand Up @@ -37,23 +36,7 @@ const defaultProps = {
};

function TaskDescriptionPage(props) {
/**
* @param {Object} values
* @param {String} values.description
* @returns {Object} - An object containing the errors for each inputID
*/
const validate = useCallback(
(values) => {
const errors = {};

if (_.isEmpty(values.description)) {
errors.description = props.translate('common.error.fieldRequired');
}

return errors;
},
[props],
);
const validate = useCallback(() => ({}), []);
dhairyasenjaliya marked this conversation as resolved.
Show resolved Hide resolved

const submit = useCallback(
(values) => {
Expand Down Expand Up @@ -89,7 +72,7 @@ function TaskDescriptionPage(props) {
<TextInput
inputID="description"
name="description"
label={props.translate('newTaskPage.description')}
label={props.translate('newTaskPage.descriptionOptional')}
dhairyasenjaliya marked this conversation as resolved.
Show resolved Hide resolved
defaultValue={(props.task.report && props.task.report.description) || ''}
ref={(el) => (inputRef.current = el)}
/>
Expand Down