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

Allow task type summary to identify task type when creating tasks in bulk #6486

Merged
merged 15 commits into from
Sep 28, 2022
Merged

Allow task type summary to identify task type when creating tasks in bulk #6486

merged 15 commits into from
Sep 28, 2022

Conversation

frcroth
Copy link
Member

@frcroth frcroth commented Sep 22, 2022 •
edited by fm3
Loading

URL of deployed dev instance (used for testing):

Steps to test (end-to-end):

  • Create tasks in bulk, try by task type id and by summary. example:
ROI2017_wkw, sampleTaskType, sampleExp, 1, 512,512,512, 0, 0, 0, 1, 500, 500, 500, 100, 100, 100, sampleProject
ROI2017_wkw, sampleTaskType, sampleExp, 1, 512,512,512, 0, 0, 0, 1, 500, 500, 500, 100, 100, 100, sampleProject
ROI2017_wkw, sampleTaskType, sampleExp, 1, 512,512,512, 0, 0, 0, 1, 500, 500, 500, 100, 100, 100, sampleProject

Note that by summary does not work if the task type summary contains commas, since that breaks the frontend csv parsing (quote escaping is not implemented there). However, we checked two large wk instances and nobody had task types with commas anyway

Steps to test (backend only):

  • Create a task via the POST /api/tasks route using a taskTypeSummary in the field taskTypeIdOrSummary rather than a taskTypeId
  • Create a task via the POST /api/tasks route using a taskTypeId to see that it still works

Issues:

@frcroth frcroth self-assigned this Sep 22, 2022
@frcroth frcroth changed the title WIP: Allow task type summary to identify task type when creating tasks in … Allow task type summary to identify task type when creating tasks in bulk Sep 22, 2022
@frcroth frcroth requested a review from fm3 September 22, 2022 11:25
@frcroth frcroth marked this pull request as ready for review September 22, 2022 11:25
fm3
fm3 approved these changes Sep 22, 2022
View reviewed changes
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Backend LGTM. @philippotto could you have a quick look at the frontend changes?

fm3
fm3 reviewed Sep 22, 2022
View reviewed changes
app/models/task/TaskCreationParameters.scala
@@ -5,7 +5,7 @@ import models.user.Experience
import play.api.libs.json.{Format, Json}

case class TaskParameters(
taskTypeId: String,
taskTypeIdOrSummary: String,
Copy link
Member

Choose a reason for hiding this comment

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

could you add a comment here explaining that after initial processing this will only contain ids?

philippotto
philippotto approved these changes Sep 23, 2022
View reviewed changes
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Overall, the changes lgtm 👍 I only added two suggestions for communicating to the user that id and summary are both valid.

However, while reading the code, I had another thought: Would it make sense to rename "Summary" to "Name"? A summary as an identifying feature seems a bit weird to me. A name would make more sense in my opinion. What do you think?

frontend/javascripts/admin/task/task_create_bulk_view.tsx Outdated Show resolved Hide resolved
frontend/javascripts/admin/task/task_create_bulk_view.tsx Outdated Show resolved Hide resolved
@fm3
Copy link
Member

fm3 commented Sep 23, 2022 •
edited
Loading

Thanks!

Would it make sense to rename "Summary" to "Name"?

Yes I think that would make sense. Not high priority, though

@philippotto
Copy link
Member

philippotto commented Sep 23, 2022 •
edited
Loading

Would it make sense to rename "Summary" to "Name"?

Yes I think that would make sense. Not high priority, though

True, but it could fit well with this PR, I think :) /edit: however, I don't know how much work this would be. so, no worries if not.

Note that by summary does not work if the task type summary contains commas, since that breaks the frontend csv parsing (quote escaping is not implemented there). However, we checked two large wk instances and nobody had task types with commas anyway

I added a form validation to the summary field so that commas may not be included for future creations/edits.

@fm3
Copy link
Member

fm3 commented Sep 23, 2022

I plan on doing a combined sql cleanup pr soon for #6465, #5944 and renaming privateLinks.accessToken to key. Maybe I can include this renaming in that evolution and propagate the new name throughout the code.

@philippotto
Copy link
Member

Ok, cool 👍

@fm3 fm3 merged commit 68cd3d5 into master Sep 28, 2022
@fm3 fm3 deleted the task-type-by-summary branch September 28, 2022 05:50
hotzenklotz added a commit that referenced this pull request Oct 13, 2022
@hotzenklotz
Merge branch 'master' of github.com:scalableminds/webknossos into pro…
8810958 
…jects-created

* 'master' of github.com:scalableminds/webknossos: (337 commits)
  Fix docs for the annotation download file format (#6546)
  Added total runtime information to VX reports (#6543)
  fix VX report for completed + skipped tasks (#6540)
  Avoid allocating spire uint objects during apply agglomerate (#6532)
  Explore remote N5 datasets (#6520)
  Fix MeshChunk byteOffset (Long, not Int) (#6536)
  update browserslist (#6505)
  Support new Mesh File (v3) (#6491)
  makes workflow_yamlContent optional (#6518)
  Always return 404 for Failures in Zarr Streaming (#6515)
  Poll wk version to notify during upgrade (#6451)
  add script which extracts newest changelog and creates GH release for it (#6504)
  release 22.10.0 (#6500)
  voxel³ -> voxel (#6501)
  Allow task type summary to identify task type when creating tasks in bulk (#6486)
  Fix sql evolution 090 (defer not null constraint) (#6498)
  SQL schema cleanup (#6492)
  Fix validation of layer selection when trying to start globalization of floodfills (#6497)
  Add "shift + w" shortcut to cycle backwards through tools (#6493)
  Fix filtering for public datasets in dataset table (#6496)
  ...
@fm3 fm3 mentioned this pull request Nov 14, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fm3 fm3 fm3 approved these changes

@philippotto philippotto philippotto approved these changes

Assignees

@frcroth frcroth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow selecting task type by name in bulk task creation
3 participants
@frcroth @fm3 @philippotto