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

Tp2000 1616 workflow task create view #1363

Open
wants to merge 8 commits into
base: TP2000-1471--task-workflow
Choose a base branch
from

Conversation

marya-shariq
Copy link
Collaborator

@marya-shariq marya-shariq commented Dec 18, 2024

TP2000-1616 : Workflow Task Create View

Why

  • As part of the task workflow management journey, users should be able to create tasks for existing workflows
  • Users can create both standalone tasks and tasks as part of workflows, thus giving them greater flexibility for task management and organisation

What

  • Two URLs for task workflow task create and confirm create
  • Two views to serve the above URLs:TaskWorkflowTaskCreate and TaskWorkflowTaskConfirmCreate
  • Both views use existing Task create form and confirm create success page templates
  • A TaskItem instance is created for each Task upon completion to link it to its corresponding workflow and manage queue position
  • Two tests - test_create_workflow_task_view and test_workflow_delete_view_deletes_related_tasks

Checklist

  • Requires migrations? - No
  • Requires dependency updates? - No

@marya-shariq marya-shariq marked this pull request as ready for review December 18, 2024 11:01
@marya-shariq marya-shariq requested a review from a team as a code owner December 18, 2024 11:01
tasks/forms.py Outdated Show resolved Hide resolved
tasks/views.py Outdated Show resolved Hide resolved
tasks/views.py Outdated Show resolved Hide resolved
name="task-workflow-task-ui-create",
),
path(
"task/confirm-create/<int:pk>",
Copy link
Collaborator

Choose a reason for hiding this comment

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

For URL pattern consistency with other confirm-create views (for instance, subtask creation uses "subtasks/<int:pk>/confirm-create/",):

Suggested change
"task/confirm-create/<int:pk>",
"task/<int:pk>/confirm-create/",

workflow=self.get_task_workflow(),
task=self.object,
)
return HttpResponseRedirect(self.get_success_url(), self.object.pk)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the second parameter to the HttpResponseRedirect constructor correct or needed?

"progress_state": progress_state.pk,
}
create_response = valid_user_client.post(create_url, form_data)
created_workflow_task = task_workflow.get_tasks().get()
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's an assert that checks for a single task count further down, so if this blows up because there isn't exactly one task instance, then that assert will never be applied. The assertion could be performed against task count before getting the (single) task.

Checking the response's status code straight after the call to post() would also usefully catch failures in a more descriptive way, rather than, say, the reverse() failing in a less straightforward way.


soup = BeautifulSoup(str(confirmation_response.content), "html.parser")

assert confirmation_response.status_code == 200
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to previous comment, before attempting to access the response's content, the assert should first perform its validation checking.

Placement of asserts in the other test, test_workflow_delete_view_deletes_related_tasks, is better because it places them close to the side-effect or invariance they're validating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants