-
Notifications
You must be signed in to change notification settings - Fork 10
Task assignment #89
base: develop
Are you sure you want to change the base?
Task assignment #89
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I just did the backend parts of the review. I'll do the frontend once all the comments are addressed as it'll be easier for me to review.
Overall, the code is good. Most things are due to the TM3->TM4 refactor and they're in different places.
There is supposed to be project level setting for enforcing task assignment that's missing; refer to (hotosm/tasking-manager@ef335b4#diff-2587407fc1a2b17abe14008d9bd0cd5b6c7ad8f0ec2321a0ba999f10ed56e32b) for that implementation.
Also, please include the migration file. Those should be committed too.
I'm set to do the front end review as soon as these items are addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the inline comments; there is a missing UI feature on the map. In the TM3 version, we used colored user-like icons on the task to show if it is assigned and if it assigned to you, it was a green logo. In TM4, let's use the same profile picture like the rest of the app. And if it's assigned to the logged in user, put a green score around the icon.
to="?status=Assigned" | ||
className={`di mh1 ${isActiveButton('Assigned', contributionsQuery)} ${linkCombo}`} | ||
> | ||
Tasks Assign to You |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need theses strings to be included the same way they are in the other <Link>
elements. You'll have to add a new object in the messages.js
file in this directory then use it here.
We'll want to be sure to do this for all strings throughout the app to ensure they're translatable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tasks Assign to You | |
Tasks Assigned to You |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Objects has been added in messages.js
|
||
{state.isError && isAssignedFound && ( | ||
<div className="bg-tan pa4 mt3"> | ||
<DataTableExtensions {...tableData} print={false}> | ||
<DataTable | ||
title="Tasks Assigned to you " | ||
columns={columns} | ||
data={data} | ||
defaultSortField="title" | ||
pagination | ||
highlightOnHover | ||
customStyles={customStyles} | ||
/> | ||
</DataTableExtensions> | ||
</div> | ||
)} | ||
{!state.isError && ( | ||
<div className={`cf db`}> | ||
<ReactPlaceholder ready={!state.isLoading} type="media" rows={10}> | ||
<TaskCards pageOfCards={state.tasks} /> | ||
</ReactPlaceholder> | ||
</div> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced data table with Task card
const columns = [ | ||
{ | ||
name: 'Project Name', | ||
selector: 'projectName', | ||
sortable: true, | ||
grow: 2, | ||
minWidth: '300px', | ||
}, | ||
{ | ||
name: 'Project', | ||
selector: 'project', | ||
sortable: true, | ||
grow: 2, | ||
minWidth: '100px', | ||
}, | ||
{ | ||
name: 'Task Id', | ||
selector: 'taskId', | ||
sortable: true, | ||
grow: 2, | ||
minWidth: '100px', | ||
cell: (row) => ( | ||
<Link to={`/projects/${row.project}/tasks?page=1&search=${row.taskId}`}> | ||
{' '} | ||
{row.taskId}{' '} | ||
</Link> | ||
), | ||
}, | ||
{ | ||
name: 'Status', | ||
selector: 'status', | ||
sortable: true, | ||
grow: 2, | ||
minWidth: '200px', | ||
}, | ||
{ | ||
name: 'Project State', | ||
selector: 'projectState', | ||
sortable: true, | ||
grow: 2, | ||
minWidth: '200px', | ||
}, | ||
]; | ||
const tableData = { | ||
columns, | ||
data, | ||
}; | ||
const customStyles = { | ||
headCells: { | ||
style: { | ||
fontSize: '15px', | ||
fontWeight: 'bold', | ||
}, | ||
}, | ||
|
||
cells: { | ||
style: { | ||
fontSize: '14px', | ||
}, | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to below; let's not use the data table and instead use the existing card style components down below. So this can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced data table with Task card
let isAssigned = state.statusCode; | ||
let isAssignedFound; | ||
if (isAssigned) { | ||
isAssignedFound = isAssigned.indexOf('ASSIGNED') !== -1 ? true : false; //true | ||
} | ||
|
||
const [userData, setUserData] = useState({}); | ||
|
||
const token = useSelector((state) => state.auth.get('token')); | ||
const userDetails = useSelector((state) => state.auth.get('userDetails')); | ||
const userName = userDetails.username; | ||
|
||
useEffect(() => { | ||
getTasks(); | ||
}, []); | ||
|
||
const getTasks = async () => { | ||
// const response = await fetchLocalJSONAPI(`user/${userName}/assigned-tasks/?closed=false`, token); | ||
const response = await fetchLocalJSONAPI( | ||
`user/${userName}/assigned-tasks/?pageSize=1000&closed=false`, | ||
token, | ||
); | ||
const jsonData = await response.assignedTasks; | ||
setUserData(jsonData); | ||
}; | ||
let data = []; | ||
|
||
for (var i = 0; i < userData.length; i++) { | ||
var obj = {}; | ||
obj.id = i + 1; | ||
obj.projectName = userData[i].projectName; | ||
obj.project = userData[i].projectId; | ||
obj.taskId = userData[i].taskId; | ||
obj.status = userData[i].taskStatus; | ||
|
||
obj.projectState = userData[i].taskStatus; | ||
data.push(obj); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be primarily for the rendering of components. Can these utilities be worked into a hook similarly to how the useTaskContributionAPI
works with this component file.
This TaskResult
component is sent a state
variable in /src/views/contributions.js
. That state
variable is populated by the useTaskContributionAPI
hook. It would probably be best if we can create a new hook, then add update the same state
variable. We should be able to check the url params to determine which hook to set the state to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now new tabs are referring Hook in UseTaskContributionAPI
@@ -88,6 +88,12 @@ export const MyTasksNav = props => { | |||
> | |||
<FormattedMessage {...messages.archived} /> | |||
</Link> | |||
<Link | |||
to="?status=Assigned" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a different param than status
since this isn't really a status. I think using assignment
is fine for the param and the values can be ASSIGNED_TO
or ASSIGNED_BY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of which, there should also be a "Tasks Assigned by You" filter that seems to be missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assignment has been added
const getUsers = useCallback((id) => { | ||
fetchLocalJSONAPI(`users/`, token) | ||
.then((res) => { | ||
setUsers(res.users); | ||
}) | ||
.catch((e) => console.log('call back failed in task index file' + e)); | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently gets all the users in the system. We should restrict it to only get users who are allowed to work on the project. This may take adding an additional API to the existing backend, I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relevant api changes are done
<div className="pr2 dib v-mid" title={msg}> | ||
<Popup | ||
trigger={ | ||
<ProfilePictureIcon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of using this icon, could you try using the user profile picture in the same place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UserAvatar is not supported by Popup,so with help of buttons same look and feel has been developed and using inside Popup
@zlavergne :pull request is ready,request you review it and merge it accordingly.