Copilot commented on code in PR #8125:
URL: https://github.com/apache/texera/pull/8125#discussion_r3890109537
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/WorkflowSearchQueryBuilder.scala:
##########
@@ -54,7 +54,8 @@ object WorkflowSearchQueryBuilder extends SearchQueryBuilder {
ownerId = WORKFLOW_OF_USER.UID,
userName = USER.NAME,
projectsOfWorkflow = groupConcatDistinct(WORKFLOW_OF_PROJECT.PID),
- workflowCoverImage =
DSL.max(WORKFLOW_COVER_IMAGE.IMAGE).as("workflow_cover_image")
+ workflowCoverImage =
DSL.max(WORKFLOW_COVER_IMAGE.IMAGE).as("workflow_cover_image"),
+ workflowIsFormView = WORKFLOW.IS_FORM_VIEW.as("workflow_is_form_view")
Review Comment:
This covers dashboard search, but the separate `GET /workflow/list` listing
still selects only the original `Record10` in
`WorkflowResource.baseWorkflowSelect` and maps that partial record into
`Workflow`. Consequently, an enabled workflow returned by that endpoint reports
the POJO default (`false`), contrary to the PR's listing/read contract. Include
`WORKFLOW.IS_FORM_VIEW` in that projection and mapping as well.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowVersionResource.scala:
##########
@@ -435,7 +435,9 @@ class WorkflowVersionResource {
assignNewOperatorIds(workflowVersion.getContent),
null,
null,
- false
+ false,
+ // the version's content carries the Form View definition, so keep
it usable
+ workflowVersion.getIsFormView
Review Comment:
The new version-clone inheritance is untested. `WorkflowVersionResourceSpec`
already exercises `cloneVersion` at lines 436–456, but only checks the ID and
name; set the source flag and assert the clone retains it, with a false case
too, so this newly promised version behavior cannot regress.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala:
##########
@@ -439,6 +442,14 @@ class WorkflowResource extends LazyLogging {
@Path("/persist")
def persistWorkflow(workflow: Workflow, @Auth sessionUser: SessionUser):
Workflow = {
val user = sessionUser.getUser
+
+ // `is_form_view` is owned by /enable-form-view and /disable-form-view
alone; a plain save
+ // sends the whole POJO to workflowDao.update, so without this its default
clears the flag.
+ if (workflow.getWid != null) {
+ Option(workflowDao.fetchOneByWid(workflow.getWid))
+ .foreach(stored => workflow.setIsFormView(stored.getIsFormView))
Review Comment:
This read-copy-write does not preserve the flag under concurrent requests.
If `/enable-form-view` commits after this fetch but before
`workflowDao.update`, the save writes the stale value back and silently undoes
the toggle. The persist update needs to exclude `IS_FORM_VIEW` (or lock and
update the row in one transaction) so the database value remains untouched
atomically.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala:
##########
@@ -726,6 +741,39 @@ class WorkflowResource extends LazyLogging {
workflowDao.update(workflow)
}
+ /**
+ * Turn the Form View on for a workflow. Only this on/off flag lives in a
column; the
+ * form's definition travels in workflow.content under `formBinding`, so
turning it
+ * off does not erase it -- toggling back on restores the author's setup.
+ */
+ @PUT
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/enable-form-view/{wid}")
+ def enableFormView(@PathParam("wid") wid: Integer, @Auth user: SessionUser):
Unit = {
+ setFormView(wid, user, enabled = true)
+ }
+
+ @PUT
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/disable-form-view/{wid}")
+ def disableFormView(@PathParam("wid") wid: Integer, @Auth user:
SessionUser): Unit = {
+ setFormView(wid, user, enabled = false)
+ }
+
+ private def setFormView(wid: Integer, user: SessionUser, enabled: Boolean):
Unit = {
+ if (!WorkflowAccessResource.hasWriteAccess(wid, user.getUid)) {
+ throw new ForbiddenException(s"You do not have permission to modify
workflow $wid")
+ }
+ // Update only this column. The flag is deliberately independent of
content, so a toggle
+ // must not rewrite the whole row -- doing so would touch content (and
could clobber a
+ // concurrent save) and bump the last-modified time for a mere flag flip.
+ context
+ .update(WORKFLOW)
+ .set(WORKFLOW.IS_FORM_VIEW, java.lang.Boolean.valueOf(enabled))
+ .where(WORKFLOW.WID.eq(wid))
+ .execute()
Review Comment:
The PR makes preserving `last_modified_time` part of the toggle contract,
but the new tests never assert it; they only check the flag and content.
Capture the timestamp before enable/disable and verify it remains unchanged, so
a later switch back to a whole-row DAO update cannot silently violate this
behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
