[No Ticket] Bulk Upload: More Task Fixes #9784
Conversation
In addition, added registration responses validation
There was a problem hiding this comment.
@fabmiz and @adlius , ready for CR. Feel free to merge if looks good so that @DougCorell can test
| if initiator_contributor.permission not in [WRITE, ADMIN]: | ||
| error = 'Initiator [{}] must at least have WRITE permission on the project [{}]'.format(initiator._id, node._id) | ||
| if initiator_contributor.permission != ADMIN: | ||
| error = 'Initiator [{}] must have admin permission on the project [{}]'.format(initiator._id, node._id) |
There was a problem hiding this comment.
Only node admin can create from node
There was a problem hiding this comment.
We need to make sure CL knows about this.
api/providers/tasks.py
Outdated
| # TODO: figure out why validation fails when `required_fields=True` | ||
| draft.validate_registration_responses(responses, required_fields=False) |
There was a problem hiding this comment.
First, draft.validate_metadata() fails as expected since the API code only log the exception.
In addition, draft.validate_registration_responses(responses, required_fields=False) actually caught an issue I had in the CSV template (empty answer to Q27).
However, draft.validate_registration_responses(responses, required_fields=True) fails even I have every cell populated.
There was a problem hiding this comment.
So what is the ValidationError thrown when required_fields=True?
There was a problem hiding this comment.
As discussed, removed.
api/providers/tasks.py
Outdated
| try: | ||
| registration = draft.register(auth, save=True) | ||
| except NodeStateError as e: | ||
| error = f'Fail to register draft: {repr(e)}' | ||
| raise RegistrationBulkCreationRowError(row.upload.id, row.id, row_title, row_external_id, error=error) | ||
| except Exception as e: | ||
| raise RegistrationBulkCreationRowError(row.upload.id, row.id, row_title, row_external_id, error=repr(e)) | ||
| logger.info('Registration [{}] created from draft [{}]'.format(registration._id, row.draft_registration._id)) | ||
|
|
||
| # Requires approval | ||
| try: | ||
| registration.require_approval(initiator) | ||
| except NodeStateError as e: | ||
| error = f'Fail to require approval: {repr(e)}' | ||
| raise RegistrationBulkCreationRowError(row.upload.id, row.id, row_title, row_external_id, error=error) | ||
| except Exception as e: | ||
| raise RegistrationBulkCreationRowError(row.upload.id, row.id, row_title, row_external_id, error=repr(e)) | ||
| logger.info('Approval required for registration [{}]'.format(registration._id)) |
There was a problem hiding this comment.
Decided to catch all exceptions just in case since the tasks don't have the roll-back on exception that the API has.
Purpose
Bulk Upload: Task Fixes
Changes
.register()and.require_approval()out of the auto-approval flow and improved exception handlingThere will be another PR for more minor improves
QA Notes
See https://www.notion.so/cos/Bugs-1802b3efdc92481ebb0fc437663cfb8f
Side Effects
N/A
Ticket
N/A