Copilot commented on code in PR #6017:
URL: https://github.com/apache/texera/pull/6017#discussion_r3628084184
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala:
##########
@@ -369,6 +372,46 @@ class WorkflowResourceSpec
assert(DashboardWorkflowEntryList.results.length == 2)
}
+ it should "return only single instance of workflow when owned and shared
publicly" in {
+ // Set workflow as public
+ testWorkflow1.setIsPublic(true)
+ // Create workflow with testUser as owner
+ workflowResource.persistWorkflow(testWorkflow1, sessionUser1)
+
+ val DashboardWorkflowEntryList =
+ dashboardResource.searchAllResourcesCall(
+ sessionUser1,
+ SearchQueryParams(),
+ includePublic = true
+ )
+ assert(DashboardWorkflowEntryList.results.length == 1)
+ assertSameWorkflow(testWorkflow1,
DashboardWorkflowEntryList.results.head.workflow.get)
+ }
Review Comment:
This test mutates the shared `testWorkflow1` fixture via
`setIsPublic(true)`. Since `testWorkflow1` is reused across the suite, this can
leak state into later tests and make the suite order-dependent. Create a
per-test `Workflow` instance instead of mutating the shared fixture.
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala:
##########
@@ -369,6 +372,46 @@ class WorkflowResourceSpec
assert(DashboardWorkflowEntryList.results.length == 2)
}
+ it should "return only single instance of workflow when owned and shared
publicly" in {
+ // Set workflow as public
+ testWorkflow1.setIsPublic(true)
+ // Create workflow with testUser as owner
+ workflowResource.persistWorkflow(testWorkflow1, sessionUser1)
+
+ val DashboardWorkflowEntryList =
+ dashboardResource.searchAllResourcesCall(
+ sessionUser1,
+ SearchQueryParams(),
+ includePublic = true
+ )
+ assert(DashboardWorkflowEntryList.results.length == 1)
+ assertSameWorkflow(testWorkflow1,
DashboardWorkflowEntryList.results.head.workflow.get)
+ }
+
+ it should "return only single instance of workflow when publicly and
explicitely shared" in {
+ // Set workflow as public
+ testWorkflow1.setIsPublic(true)
+ // Create workflow with testUser as owner
+ val savedWorkflow = workflowResource.persistWorkflow(testWorkflow1,
sessionUser1)
+ // Share workflow with write acccess to testUser2
+ val workflowAccessResource = new WorkflowAccessResource()
+ workflowAccessResource.grantAccess(
+ savedWorkflow.getWid,
+ testUser2.getEmail,
+ "READ",
+ sessionUser1
+ )
+
+ val DashboardWorkflowEntryList =
+ dashboardResource.searchAllResourcesCall(
+ sessionUser2,
+ SearchQueryParams(),
+ includePublic = true
+ )
+ assert(DashboardWorkflowEntryList.results.length == 1)
+ assertSameWorkflow(savedWorkflow,
DashboardWorkflowEntryList.results.head.workflow.get)
+ }
Review Comment:
This test also mutates the shared `testWorkflow1` fixture via
`setIsPublic(true)`, which can leak state into other tests. Additionally, the
test description/comment contain typos ("explicitely", "acccess") and the
comment says "write" while the test grants READ. Prefer a per-test `Workflow`
instance and correct the wording to keep the intent clear.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/WorkflowSearchQueryBuilder.scala:
##########
@@ -150,9 +153,9 @@ object WorkflowSearchQueryBuilder extends
SearchQueryBuilder {
val pidField = groupConcatDistinct(WORKFLOW_OF_PROJECT.PID)
val dw = DashboardWorkflow(
record.into(WORKFLOW_OF_USER).getUid.eq(uid),
Review Comment:
`DashboardWorkflow.isOwner` is computed using `.eq(uid)`, which is reference
equality on boxed `Integer` and can incorrectly return false even when the
numeric values match. Use Scala value equality (`==`) to compare the IDs.
--
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]
