-
Notifications
You must be signed in to change notification settings - Fork 7
Reimplement Workflow with its own schema #7203
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
Conversation
…sk RowId instead of exp.ProtocolApplication
XingY
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSampleWorkflowProtocol and related utils in ExpProtocol are still being used. I believe those should be removed.
|
|
||
| public NameExpressionDataIteratorBuilder(DataIteratorBuilder pre, TableInfo parentTable) | ||
| { | ||
| _pre = pre; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can just call this(pre, parentTable, "name", "nameExpression")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes. Done.
| String workflowObjectId = Lsid.parse(workflowTask.getLSID()).getObjectId(); | ||
| xRun.setWorkflowTaskLSID("${WorkflowTaskReference}:" + workflowObjectId); | ||
| } | ||
| // TODO need to update this for run workflowTaskId support in XAR on workflow job folder export/import is supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for a future story?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Follow on story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR reimplements workflow functionality by moving it from the Exp (experiment) schema to a dedicated workflow schema in the samplemanagement module. The changes streamline the API by replacing complex object relationships with simpler ID-based references.
Key changes:
- Refactored workflow task references from
ExpProtocolApplicationobjects toLongIDs throughout the assay and experiment APIs - Enhanced
CreatedModifiedclass with additional convenience getters/setters for Date and User objects - Made
AbstractQueryUpdateServicepermission checks overridable to support custom authorization logic - Extracted
PageFlowUtil.AppUrlsas a reusable base class for application URL providers - Moved
NamePlusIdDataIteratorfrom assay.plate.query to the api.dataiterator package and parameterized name generation components for broader reusability
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| experiment/src/org/labkey/experiment/api/ExpRunTableImpl.java | Updates WorkflowTask column FK to reference new workflow.Task schema with module check |
| experiment/src/org/labkey/experiment/api/ExpRunImpl.java | Simplifies workflow task methods to return/accept IDs instead of objects |
| experiment/src/org/labkey/experiment/XarReader.java | Comments out workflow task LSID handling pending XAR format update |
| experiment/src/org/labkey/experiment/XarExporter.java | Comments out workflow task export pending XAR format update |
| experiment/src/org/labkey/experiment/ExperimentModule.java | Bumps schema version to 25.014 |
| experiment/src/org/labkey/experiment/ExperimentMigrationSchemaHandler.java | Removes FK constraint management for workflow task (moved to SQL scripts) |
| experiment/resources/schemas/dbscripts/sqlserver/exp-25.013-25.014.sql | Drops FK_Run_WorfklowTask constraint |
| experiment/resources/schemas/dbscripts/postgresql/exp-25.013-25.014.sql | Drops FK_Run_WorfklowTask constraint |
| assay/src/org/labkey/assay/plate/query/PlateTable.java | Adds import for relocated NamePlusIdDataIterator |
| assay/src/org/labkey/assay/plate/query/PlateSetTable.java | Adds import for relocated NamePlusIdDataIterator |
| assay/src/org/labkey/assay/actions/ImportRunApiAction.java | Renames workflowTask variables to workflowTaskId |
| api/src/org/labkey/api/util/PageFlowUtil.java | Introduces AppUrls base class for application URL generation |
| api/src/org/labkey/api/query/AbstractQueryUpdateService.java | Adds overridable permission check methods for insert/update/delete operations |
| api/src/org/labkey/api/qc/TsvDataExchangeHandler.java | Removes unused getWorkflowTask method override |
| api/src/org/labkey/api/exp/api/ExpRun.java | Replaces workflow task object methods with ID-based methods |
| api/src/org/labkey/api/dataiterator/NamePlusIdDataIterator.java | Relocates from assay.plate.query package to api.dataiterator |
| api/src/org/labkey/api/dataiterator/NameExpressionDataIteratorBuilder.java | Adds parameterized constructor for custom column names |
| api/src/org/labkey/api/dataiterator/NameExpressionDataIterator.java | Adds parameterized constructor for custom column names |
| api/src/org/labkey/api/data/NameGeneratorState.java | Adds nameField parameter for flexible name field lookup |
| api/src/org/labkey/api/data/NameGenerator.java | Adds createState overload with nameField parameter |
| api/src/org/labkey/api/data/CreatedModified.java | Adds Date and User convenience getters, and int-based setters |
| api/src/org/labkey/api/assay/pipeline/AssayUploadPipelineJob.java | Updates to use getWorkflowTaskId method |
| api/src/org/labkey/api/assay/pipeline/AssayRunAsyncContext.java | Renames field and methods from workflowTask to workflowTaskId |
| api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java | Renames field and methods from workflowTask to workflowTaskId |
| api/src/org/labkey/api/assay/DefaultAssayRunCreator.java | Updates to use getWorkflowTaskId method |
| api/src/org/labkey/api/assay/AssayRunUploadContextImpl.java | Renames field from workflowTask to workflowTaskId |
| api/src/org/labkey/api/assay/AssayRunUploadContext.java | Renames method from getWorkflowTask to getWorkflowTaskId |
| api/src/org/labkey/api/assay/AssayRunDatabaseContext.java | Simplifies to directly return workflow task ID |
| api/src/org/labkey/api/announcements/EmailOption.java | Renames enum constants from SAMPLEMANAGER_* to WORKFLOW_* |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@labkey-susanh My main comment for this PR is updating/removing usages of isSampleWorkflowProtocol and related. |
Yes, sorry. Asked for re-review too early. |
I have removed references to this except where currently in use by the folder writer/reader. Those will get excised when we do the folder export/import part but I am choosing to leave them for now as a trail to follow in that next story. |
Rationale
Having our workflow implementation based on the Exp schema leads to lots of inefficiencies and makes it difficult to expand the capabilities. The related PR moves the data into dedicated tables in the database, producing a new rendition of the
workflowschema. The changes here make that transition better.Related Pull Requests
Changes
CreatedModifiedAbstractQueryUpdateServiceto have overridable methods for checking permissionsPageUtils.AppUrlsclass as a base class for URL providers for apps (extracted fromNotebookController)