Show active users modal #2863

MichaelBuessemeyer · 2018-07-09T07:54:37Z

Mailable description of changes:

Admins now can see and transfer all active tasks of a project to a single user

URL of deployed dev instance (used for testing):

Steps to test:

go to the admin -> projects
use the show active users button on the right side to open the modal
check the table if its showing all users with their active tasks
select a user and then transfer the active tasks to the user (a toast should show up to confirm the transfer)
then check the tasks-tab from the dashboard to confirm the transfer

Issues:

Ready for review

…ects-view that displays all users with open tasks of the selected project in a modal

…d a modified copy of the task_transfer_modal to control a bulk task transfer

…hold, waiting for backend to provide the api request

…real functionallity

jstriebel · 2018-07-09T08:21:06Z

@MichaelBuessemeyer: Please add the change notes from this PR to the changelog

…ected

…-api-request-for-open-tasks

…w-active-users-modal

…bknossos into add-api-request-for-open-tasks

daniel-wer · 2018-07-11T09:33:35Z

CHANGELOG.md

-  - Added permission for team managers to refresh datasets. [#2688](https://github.com/scalableminds/webknossos/pull/2688)
-  - Added backend-unit-test setup and a first test for NML validation. [#2829](https://github.com/scalableminds/webknossos/pull/2829)
-
+* Added a button to the administration/project-tab, that displays a modal with all users with active annotations of the selected project. All these annotations can be transfered to a single user [#2670](https://github.com/scalableminds/webknossos/issues/2670)


@MichaelBuessemeyer You accidently re-formatted the whole CHANGELOG.md file (or better, the prettier sublime plugin did). Please revert the changes in this file, add the following to your JsPrettier sublime user config: "auto_format_on_save_excludes": ["*/node_modules/*","*.md"] (so .md files are no longer "prettified", and then re-add your changelog entry :)

…to display the table in the modal

…tive tasks of a project, transfer modal only renders if set to visible

…and change api call to post-type

…bknossos into show-active-users-modal

…to messages

philippotto

Awesome, nice work, @MichaelBuessemeyer! The code looks really good and idiomatic 👍

Nevertheless, I made a few remarks on how to improve the code even further. Most of them are minor things and should be quick to fix, so don't feel overwhelmed :)

I'll test after the next iteration!

philippotto · 2018-07-13T12:52:06Z

app/assets/javascripts/admin/project/project_list_view.js

+    });
+  };
+
+  transferCommited = () => {


I'd rename this to onTaskTransferComplete.

The function is not used at the moment, right? Shouldn't it be called when the modal is closed?

philippotto · 2018-07-13T12:53:27Z

app/assets/javascripts/admin/project/project_list_view.js

@@ -283,6 +302,13 @@ class ProjectListView extends React.PureComponent<Props, State> {
              />
            </Table>
          </Spin>
+          {this.state.isTransferTasksVisible ? (
+            <TransferAllTasksModal
+              visible={this.state.isTransferTasksVisible}


Due to the ternary from two lines above, this prop will always be true. You can remove it.

philippotto · 2018-07-13T12:55:47Z

app/assets/javascripts/admin/project/transfer_all_tasks_modal.js

+
+type Props = {
+  project: ?APIProjectType,
+  onCancel: Function,


Can you use () => void instead of Function? This is more precise, as it specifies the types of the variables and the return value.

philippotto · 2018-07-13T12:57:12Z

app/assets/javascripts/admin/project/transfer_all_tasks_modal.js

+    if (this.props.project) {
+      usersWithActiveTasks = await getUsersWithActiveTasks(this.props.project.name);
+    }
+    this.setState({ isLoading: false, usersWithActiveTasks });


You can combine this setState call with the one in line 56.

philippotto · 2018-07-13T12:58:31Z

app/assets/javascripts/admin/project/transfer_all_tasks_modal.js

+  }
+
+  async fetchData() {
+    this.setState({ isLoading: true });


Please structure this function like this so that potential errors from the server, won't freeze the view.

try { this.setState({ isLoading: true }); // .... await ... } except (error) { handleGenericError(error); } finally { this.setState({isLoading: false}); }

Otherwise, isLoading would never be set to false if a request fails. handleGenericError can be imported from error_handling.js.

philippotto · 2018-07-13T13:15:36Z

app/assets/javascripts/admin/project/transfer_all_tasks_modal.js

+    }
+    const project = this.props.project;
+    if (!project) {
+      Toast.error(messages["project.none_selected"]);


When does this happen? Showing a toast is a side effect (if the render function is called twice and this block is executed, there will be two toast notifications). In general, side effects should be avoided within the render function, as it can be executed various times. In this specific scenario, I would just remove the toast.error line. Instead leave the return null line.

philippotto · 2018-07-13T13:16:40Z

app/assets/javascripts/admin/project/transfer_all_tasks_modal.js

+      Toast.error(messages["project.none_selected"]);
+      return null;
+    } else {
+      Toast.close(messages["project.none_selected"]);


Same here. If you would like to show such message (when does this happen?), you can just return the message, as this will be the content of the modal.

philippotto · 2018-07-13T13:17:19Z

app/assets/javascripts/admin/project/transfer_all_tasks_modal.js

+      return (
+        <Modal
+          title={title}
+          visible={this.props.visible}


Since the modal will always be visible when this component is rendered you can just write <Modal title={title} visible onCancel=....

philippotto · 2018-07-13T13:19:55Z

app/assets/javascripts/admin/project/transfer_all_tasks_modal.js

+              >
+                Transfer all tasks
+              </Button>
+              <Button onClick={() => this.props.onCancel()}>Close</Button>


You can just write onClick={this.props.onCancel}. The arrow function would be important, if the context is important. However, the parent component has to take care of that, anyway, since you are getting that value from props.

In line 164, you could also write onClick={this.transferAllActiveTasks} if you define the corresponding function like transferAllActiveTasks = async () => {.... However, the current way is also fine :)

philippotto · 2018-07-13T13:21:20Z

app/assets/javascripts/messages.js

@@ -90,6 +90,11 @@ In order to restore the current window, a reload is necessary.`,
  "project.delete": "Do you really want to delete this project?",
  "project.increase_instances":
    "Do you really want to add one additional instance to all tasks of this project?",
+  "project.none_selected": "No currently selected project found",


Please make sure that the messages are ended with a fullstop.

philippotto · 2018-07-13T13:26:07Z

@fm3 Can you review the backend code?

philippotto · 2018-07-13T13:31:29Z

app/assets/javascripts/admin/project/transfer_all_tasks_modal.js

+
+  renderFormContent() {
+    return (
+      <Select


You copied that code from the transfer_task_modal, right? If you feel like it, you can share this code by creating a new <UserSelectionComponent /> component. The approach would be very similar to the TeamSelectionComponent which abstracts the same thing for teams. If you think it's to much for this PR, please add a comment above this line here (e.g., "Todo: create a component which is used here and in transfer_task_modal").

fm3

Backend LGTM

philippotto

I made some last, small remarks :) Will test now!

philippotto · 2018-08-09T15:37:41Z

CHANGELOG.md

@@ -18,8 +18,9 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.md).
 - Added shortcuts for moving along the current tracing direction in orthogonal mode. Pressing 'e' (and 'r' for the reverse direction) will move along the "current direction", which is defined by the vector between the last two created nodes.
 - Added a banner to the user list to notify admins of new inactive users that need to be activated. [#2994](https://github.com/scalableminds/webknossos/pull/2994)
 - When a lot of changes need to be persisted to the server (e.g., after importing a large NML), the save button will show a percentage-based progress indicator.
- Added placeholders and functionality hints to (nearly) empty lists and tables in the admin views. [#2969](https://github.com/scalableminds/webknossos/pull/2969)
+- Added the possibility for admins to see and transfer all active tasks of a project to a single user in the porject tab[#2863](https://github.com/scalableminds/webknossos/pull/2863)


typo in project :)

philippotto · 2018-08-09T15:42:25Z

app/assets/javascripts/admin/project/transfer_all_tasks_modal.js

+      });
+    } catch (error) {
+      handleGenericError(error);
+    }


Please add an isLoading attribute to the component's state. This attribute should be set to true before fetching, and set to false in a finally block after catch. Similar to:

try { this.setState({ isLoading: true, }); const updatedProject = await increaseProjectTaskInstances(project.name); this.setState({ projects: this.state.projects.map(p => (p.id === project.id ? updatedProject : p)), }); } catch (error) { handleGenericError(error); } finally { this.setState({ isLoading: false }); }

Then, use the isLoading attribute to render a spin component. Similar to

render() { return <Spin spinning={this.state.isLoading} size="large"> ... </Spin>; }

philippotto · 2018-08-10T11:13:44Z

Not sure how I managed to do this, but I think something is not right with the back-end part. @fm3 Can you have a look?

I created a second user and a second project B. For both projects I created some tasks, and requested them for both users. Afterwards, I transferred the tasks. For the task of project B, the tasks route does give me an annotation, but the task attribute is null?

This leads to an error message (front-end says: "Annotation 5b6d70a27d0000d100b4f2bf has no task assigned. Please inform your admin.") and none of the tasks are rendered in the task lists because of that.

When I transfer all the tasks back to scmboy, I don't get the error anymore. Only the one task of project B is causing this 🤔

fm3 · 2018-08-13T07:06:22Z

@philippotto Good catch! This happens when the user has no read access to the annotation’s dataset. I just pushed a commit that forbids this transfer entirely.
Do we expect to merge this today? Because part of that bug exists in master, so I would create a separate PR to fix it faster.

philippotto

Awesome 👍 Should be good to go, now.

@fm3 This PR doesn't need a special deployment (migration / datastore update), right?

philippotto · 2018-08-13T08:12:19Z

Just tested it and there is a comprehensible error message 👍 It might be useful to say "transfer all tasks which whose datasets are accessible for the user", but I'd wait until this feature is actually requested from the lab.

fm3 · 2018-08-13T08:24:26Z

This PR doesn't need a special deployment (migration / datastore update), right?

yes, does not contain datastore or schema changes :)

MichaelBuessemeyer added 6 commits July 4, 2018 18:04

Added usersWithOpenTasks to the admin api. Added a button to the proj…

4c0ccc9

…ects-view that displays all users with open tasks of the selected project in a modal

added method to transfer many tasks at once to admin_rest_api, create…

c65843c

…d a modified copy of the task_transfer_modal to control a bulk task transfer

adding a fetch function, that fetches all users with open tasks -> on…

18129e5

…hold, waiting for backend to provide the api request

creating mock data, ongoing

a94b18f

added mock data, the modal is now working and displayable but has no …

d285460

…real functionallity

fixed flow errors

93bce49

MichaelBuessemeyer and others added 8 commits July 9, 2018 10:52

fixed flow errors and added some error handling if no project was sel…

f31013b

…ected

[WIP] transfer annotations to different user for api #2862

a3acacf

Merge branch 'master' of github.com:scalableminds/webknossos into add…

5628221

…-api-request-for-open-tasks

[WIP] route for transferring active tasks of a project to a user #2862

df84522

routes for showing active tasks and transfering them #2862

b88a2ea

Merge branch 'master' of github.com:scalableminds/webknossos into sho…

0aa6baf

…w-active-users-modal

Merge branch 'show-active-users-modal' of github.com:scalableminds/we…

16c6f52

…bknossos into add-api-request-for-open-tasks

added changelog, moved error message to messages, fixed flow error

7c958f5

fm3 added backend frontend labels Jul 10, 2018

fm3 assigned MichaelBuessemeyer Jul 10, 2018

daniel-wer reviewed Jul 11, 2018

View reviewed changes

MichaelBuessemeyer and others added 10 commits July 12, 2018 01:05

added the request for active users to the admin_rest_api and used it …

9c21832

…to display the table in the modal

added a type for activeUsers, added an api request to transfer all ac…

7fc64d3

…tive tasks of a project, transfer modal only renders if set to visible

reverted changelog -> entry for this issue still missing

923aaba

added changelog entry

586f21d

Merge branch 'master' into show-active-users-modal

f368318

added message for successful transfer, added transfer functionality, …

dd9c02b

…and change api call to post-type

Merge branch 'show-active-users-modal' of github.com:scalableminds/we…

1bf3214

…bknossos into show-active-users-modal

corrected changelog entry

6ea5366

added error handling to the transfer request, added an error message …

69ca333

…to messages

refeshed snapshots

3d0c911

philippotto changed the title ~~[WIP] Show active users modal~~ Show active users modal Jul 13, 2018

philippotto requested changes Jul 13, 2018

View reviewed changes

philippotto requested a review from fm3 July 13, 2018 13:26

philippotto reviewed Jul 13, 2018

View reviewed changes

fm3 approved these changes Jul 15, 2018

View reviewed changes

fm3 mentioned this pull request Jul 17, 2018

Add API requests for showing active tasks and transferring them #2868

Merged

3 tasks

philippotto added this to the Sprint 25a milestone Jul 19, 2018

merge master into show-active-users-modal

330e19c

philippotto modified the milestones: Sprint 25a, Sprint 24a Aug 6, 2018

MichaelBuessemeyer added 5 commits August 9, 2018 16:21

refactored requested changes

c00e535

resolved conflicts

b3e83a7

updated snapshots and resolved auto merging errors

464ac2e

fixed flow type problems

f3c31b8

fixed linter issues

970f3d7

philippotto reviewed Aug 10, 2018

View reviewed changes

MichaelBuessemeyer and others added 2 commits August 10, 2018 14:29

added a spinner while fetching data

0a2aba0

disallow transferring tasks to users who cannot access the dataset

6b5b4af

philippotto approved these changes Aug 13, 2018

View reviewed changes

fm3 merged commit d099fd7 into master Aug 13, 2018

philippotto deleted the show-active-users-modal branch August 13, 2018 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show active users modal #2863

Show active users modal #2863

MichaelBuessemeyer commented Jul 9, 2018 •

edited by fm3

Loading

jstriebel commented Jul 9, 2018

daniel-wer Jul 11, 2018

philippotto left a comment

philippotto Jul 13, 2018

philippotto Jul 13, 2018

philippotto Jul 13, 2018

philippotto Jul 13, 2018

philippotto Jul 13, 2018

philippotto Jul 13, 2018

philippotto Jul 13, 2018

philippotto Jul 13, 2018

philippotto Jul 13, 2018

philippotto Jul 13, 2018

philippotto Jul 13, 2018

philippotto commented Jul 13, 2018

philippotto Jul 13, 2018 •

edited

Loading

fm3 left a comment

philippotto left a comment

philippotto Aug 9, 2018

philippotto Aug 9, 2018

philippotto Aug 9, 2018

philippotto commented Aug 10, 2018

fm3 commented Aug 13, 2018

philippotto left a comment

philippotto commented Aug 13, 2018

fm3 commented Aug 13, 2018

Show active users modal #2863

Show active users modal #2863

Conversation

MichaelBuessemeyer commented Jul 9, 2018 • edited by fm3 Loading

Mailable description of changes:

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

jstriebel commented Jul 9, 2018

Choose a reason for hiding this comment

philippotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippotto commented Jul 13, 2018

philippotto Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

fm3 left a comment

Choose a reason for hiding this comment

philippotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippotto commented Aug 10, 2018

fm3 commented Aug 13, 2018

philippotto left a comment

Choose a reason for hiding this comment

philippotto commented Aug 13, 2018

fm3 commented Aug 13, 2018

MichaelBuessemeyer commented Jul 9, 2018 •

edited by fm3

Loading

philippotto Jul 13, 2018 •

edited

Loading