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.

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

Add all csv info to displayed table on successful task creation #5491

Merged
merged 10 commits into from
Jun 1, 2021

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented May 15, 2021

This PR unifies the information that is displayed in the table that is shown upon successful task creating with the information of the CSV.

Imo this change does not make so much sense. Maybe I misunderstood something but to me, the display information in the modal is far too much to be easily understood/is not clearly arranged. Additionally, the file name might get too huge if all IDs (like suggested) are added to the file name. Thus I limited this to a maximum of 3 task ids.

I think some general input on my points is needed before we start a code review :)

URL of deployed dev instance (used for testing):

Steps to test:

  • Got to admin->projects and add a new project with the name testProject.
  • Got to the admin->task view
  • Open the task creation in bulk version.
  • Use the following for the bulk creation:
ROI2017_wkw,609fca97c701002702e9eb9c,sampleExp,1,0,0,0,0,0,0,1,0,0,0,1024,1024,1024,sampleProject
ROI2017_wkw,609fca97c701002702e9eb9c,sampleExp,1,0,0,0,0,0,0,1,0,0,0,1024,1024,1024,testProject
  • The modal that opens up should first display warnings. As the bulk creation above creates tasks for multiple projects, the following should be included in the warnings: You created tasks for multiple projects at a time: sampleProject, testProject. Below the error list there should be the CSV download button and then the short list for successfully created tasks.
  • The name of the downloadable csv should be something like: task_id_sampleProject_testProject_2021-05-21_14-43.csv.
  • If you do another bulk task creation but this time for only one project, the warning mentioned above should not appear and the csv filename should only include that project's name.

Issues:


@MichaelBuessemeyer
Copy link
Contributor Author

@fm3 As you created the issue for this: Do you have an opinion about my thought about whether the changes actually make sense? Or do you have some suggestions to improve these changes?

@fm3
Copy link
Member

fm3 commented May 17, 2021

Thanks for looking into this! You are quite right, these changes create somewhat weird behavior. I wrote a message to the original requesting user asking to clarify, let’s see what they reply :) Should have done that immediatly when writing the issue, I’m sorry if this created redundant work!

@daniel-wer daniel-wer removed their request for review May 19, 2021 09:45
@daniel-wer
Copy link
Member

The expected changes have been clarified, see https://discuss.webknossos.org/t/show-task-ids-for-created-tasks-miss-filename/1621/6.
Please request a review once you incorporated those :)

@fm3
Copy link
Member

fm3 commented May 19, 2021

tbh I’d slightly deviate from the IDs_-prefix suggested there, and go with the more general tasks_ids_<projectnames>_<date>.csv or something like that.

I also don’t like hiding the short success list, as I still believe it to be useful when creating few tasks. But maybe we can make the CSV download button more prominent even if the short list is there. And maybe the list can be styled differently to more obviously be something different than the CSV. That way, users who want to use the CSV workflow can always do that, and users who like instant feedback on (few) created tasks can still get it. What do you think?

@daniel-wer
Copy link
Member

tbh I’d slightly deviate from the IDs_-prefix suggested there, and go with the more general tasks_ids__.csv or something like that.

Sounds fair to me.

I also don’t like hiding the short success list, as I still believe it to be useful when creating few tasks. But maybe we can make the CSV download button more prominent even if the short list is there. And maybe the list can be styled differently to more obviously be something different than the CSV. That way, users who want to use the CSV workflow can always do that, and users who like instant feedback on (few) created tasks can still get it. What do you think?

It could be hidden behind a "Details" toggle, like this to de-clutter the popup by default?
details-toggle

Also, from the post it sounded like the main issue was that the CSV download button was only visible if a lot of tasks were created. So that should be changed to be always visible.

@fm3
Copy link
Member

fm3 commented May 19, 2021

It could be hidden behind a "Details" toggle, like this to de-clutter the popup by default?

Yes, looks nice :) but that might not even be necessary if the real problem is the accessability of the CSV download button.

Also, from the post it sounded like the main issue was that the CSV download button was only visible if a lot of tasks were created. So that should be changed to be always visible.

I agree. I think it is always visible already, but should be moved above the list, so the user does not have to scroll to get there.

@MichaelBuessemeyer
Copy link
Contributor Author

Thank you two for already discussing the issue 👍.
I'll try to implement your ideas :)

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented May 21, 2021

I just pushed the following changes:

  • I updated the csv file name according to @fm3 suggestion and removed the spaces in the date formatting.
  • I made the Download CSV button more prominent.
  • Added a warning when creating tasks for multiple projects at once

With that I think I covered all points @daniel-wer :)

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Very nice, works well 👍

How would I go about testing the "failed" path, so that some of the tasks fail during creation? If I change the dataset name, taskTypeId, or projectName I only end up with error toasts, but not with the popup and "the creation of x tasks failed".

Also, I noticed that when I try to create a task for a project that is not existing, I end up with an unhelpful error toast Couldn't find the requested resource. without further details. @fm3 Would it be easy to improve that? Otherwise, I'll create a follow up issue :)

frontend/javascripts/admin/task/task_create_form_view.js Outdated Show resolved Hide resolved
@fm3
Copy link
Member

fm3 commented May 27, 2021

@daniel-wer I’ll have a look at the too-generic message, but I’d still track it in a separate issue.

To add a failed task to the report, you can modify line 426 in TaskCreationService.scala to result <- TaskCreationResult.fromTaskJsFoxes(Fox.failure("Nope") :: taskJsons, warnings) (prepends a failure to the result list)

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Great, works very well 👍

@MichaelBuessemeyer MichaelBuessemeyer enabled auto-merge (squash) June 1, 2021 13:16
@MichaelBuessemeyer MichaelBuessemeyer merged commit e8f9291 into master Jun 1, 2021
@fm3 fm3 deleted the unify-task-table-with-csv branch June 7, 2021 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify task creation CSV with displayed table
3 participants