FLPATH-313: get workflow by projectId #294

RichardW98 · 2023-05-01T19:48:41Z

FLPATH-313: get workflow by projectId

Change type

Impacted services

Workflow Service
Notifivcation Service

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.

openshift-ci-robot · 2023-05-01T19:48:44Z

openshift-ci-robot · 2023-05-01T19:48:47Z

eloycoto · 2023-05-02T08:04:22Z

...e/src/test/java/com/redhat/parodos/workflow/execution/controller/WorkFlowControllerTest.java

@@ -231,6 +234,22 @@ public void TestGetStatusWithValidData() throws Exception {
 				.andExpect(MockMvcResultMatchers.status().isOk());
 	}

+	@Test
+	void TestGetStatusByProjectIdWithValidData() throws Exception {


I miss the following test:

where checks that the project is nill? Does it return all projects?

What happens is getWorkFlowStatusByProjectId return an empty list?

yes it should return all projects which the user is allowed to check

if it returns empty list then UI won't show any executions in the workflow table

I will add tests for them

eloycoto · 2023-05-02T08:05:22Z

...ice/src/test/java/com/redhat/parodos/workflow/execution/service/WorkFlowServiceImplTest.java

@@ -907,6 +909,27 @@ public void testGetWorkflowParametersWithoutWorkflowOptions() {
 		assertNull(workflowParameters.getWorkFlowOptions().getNewOptions());
 	}

+	@Test
+	void getWorkFlowStatusByProjectId_when_projectIsFound_then_returnWorkFlowStatus() {


We need to increase testing when no workflows with valid projects and what happens when JPA repo returns nil

eloycoto · 2023-05-02T08:06:13Z

...service/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowServiceImpl.java

+	@Override
+	public List<WorkFlowStatusResponseDTO> getWorkFlowStatusByProjectId(UUID projectId) {
+		return workFlowRepository.findAllByProjectId(projectId).stream()
+				.map(workFlowExecution -> getWorkFlowStatus(workFlowExecution.getId())).toList();


Why is the reason for this map?
Could you add a test for this?

@eloycoto stream().map() allows applying a function to each stream element. So for me, it makes sense to iterate over the element and call getWorkflowStatus and then collect all results into a List.
However, I agree that a unit test for this method is valuable.

we have unit tests for getWorkFlowStatus(): https://github.com/RichardW98/parodos/blob/filter-workflow-by-projectid/workflow-service/src/test/java/com/redhat/parodos/workflow/execution/service/WorkFlowServiceImplTest.java#L444

masayag · 2023-05-02T09:21:47Z

...rvice/src/main/java/com/redhat/parodos/workflow/execution/controller/WorkFlowController.java

+					@ApiResponse(responseCode = "403", description = "Forbidden", content = @Content) })
+	@GetMapping("/status")
+	public ResponseEntity<List<WorkFlowStatusResponseDTO>> getStatusByProjectId(
+			@RequestParam("projectId") String projectId) {


projectId should be marked as @NotNull

masayag · 2023-05-02T09:26:44Z

...rvice/src/main/java/com/redhat/parodos/workflow/execution/controller/WorkFlowController.java

+					@ApiResponse(responseCode = "401", description = "Unauthorized", content = @Content),
+					@ApiResponse(responseCode = "403", description = "Forbidden", content = @Content) })
+	@GetMapping("/status")
+	public ResponseEntity<List<WorkFlowStatusResponseDTO>> getStatusByProjectId(


not sure what is the reason for it, but the generated openapi.json returns a string as a result while I'd expect the type of the return value to be a list.

however, if the project is mandatory in this context, I wonder why not expose the project-status endpoint instead.

if the purpose is to evaluate the project status, then it seems more natural to query for the project resource instead of the workflow, as project status might contain other information relevant to the project's overall status.

What will be the use of the new endpoint?

Since this method return the list of the statuses of all the workflows available in a project (if I understood correctly), maybe renaming the method into getStatusesByProjectId can ease the understanding.

@gciavarrini @masayag new UI design has replaced the project Overview with workflows Overview (https://www.figma.com/file/iu0HAOSoLOESA2GHyxuHLg/parados-mui?node-id=3640-3490). Therefore project-status will not be needed any more, and instead we will need to show all workflow executions' info by project. I will update this pr to include data other than status in the return (e.g. startTime, endTime ...). @wKich has a pr for this in UI: rhdhorchestrator/backstage-parodos#124. please review

gciavarrini · 2023-05-02T09:07:13Z

...e/src/test/java/com/redhat/parodos/workflow/execution/controller/WorkFlowControllerTest.java

+						projectId.toString()))
+				.andExpect(MockMvcResultMatchers.status().isOk())
+				.andExpect(MockMvcResultMatchers.content().contentType(MediaType.APPLICATION_JSON))
+				.andExpect(MockMvcResultMatchers.jsonPath("$", hasSize(1))).andExpect(MockMvcResultMatchers


not sure if a formatter issue: can you move the last andExpect to a new line?

yea. it's formatter 😅

gciavarrini · 2023-05-02T09:11:39Z

...service/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowServiceImpl.java

+	@Override
+	public List<WorkFlowStatusResponseDTO> getWorkFlowStatusByProjectId(UUID projectId) {
+		return workFlowRepository.findAllByProjectId(projectId).stream()
+				.map(workFlowExecution -> getWorkFlowStatus(workFlowExecution.getId())).toList();


@eloycoto stream().map() allows applying a function to each stream element. So for me, it makes sense to iterate over the element and call getWorkflowStatus and then collect all results into a List.
However, I agree that a unit test for this method is valuable.

gciavarrini · 2023-05-02T09:34:48Z

...rvice/src/main/java/com/redhat/parodos/workflow/execution/controller/WorkFlowController.java

+					@ApiResponse(responseCode = "401", description = "Unauthorized", content = @Content),
+					@ApiResponse(responseCode = "403", description = "Forbidden", content = @Content) })
+	@GetMapping("/status")
+	public ResponseEntity<List<WorkFlowStatusResponseDTO>> getStatusByProjectId(


Since this method return the list of the statuses of all the workflows available in a project (if I understood correctly), maybe renaming the method into getStatusesByProjectId can ease the understanding.

masayag · 2023-05-03T13:42:02Z

workflow-service/src/main/java/com/redhat/parodos/project/repository/ProjectRepository.java

@@ -26,4 +29,8 @@
 */
 public interface ProjectRepository extends JpaRepository<Project, UUID> {

+	List<Project> findAllByUserUsername(String username);


pls add tests.
should be fairly simple with https://github.com/parodos-dev/parodos/pull/295/files#diff-43f3718db35688f81c93147df5fad5e6caf4bbdc988a24d007edd32db7a89372 (once it is being merged)

yes Thank you for the example ~

RichardW98 · 2023-05-03T20:27:38Z

I renamed 'user' table to 'users' temporarily, will fix it in another pr to standandize table names

masayag · 2023-05-03T13:42:37Z

workflow-service/src/main/java/com/redhat/parodos/project/service/ProjectService.java

@@ -33,4 +35,8 @@ public interface ProjectService {

 	List<ProjectResponseDTO> getProjects();

+	List<ProjectResponseDTO> findProjectsByUserName(String userName);


for consistency, let's keep userName in lowercase

masayag · 2023-05-03T13:45:29Z

workflow-service/src/main/java/com/redhat/parodos/security/SecurityUtils.java

+			ldapDetails = (LdapUserDetailsImpl) SecurityContextHolder.getContext().getAuthentication().getPrincipal();
+			return ldapDetails.getUsername();
+		}
+		catch (NullPointerException e) {


what is expected to generate the NPE?
Could we replace this with if instead of try-catch?

is NPE related to ldapDetails? If so we could check instead of handling this exception

SecurityContextHolder.getContext().getAuthentication() might be null. I will change it to if/else

workflow-service/generated/openapi/openapi.json

masayag · 2023-05-04T07:27:28Z

...service/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowServiceImpl.java

@@ -172,6 +185,26 @@ public synchronized WorkFlowExecution updateWorkFlow(WorkFlowExecution workFlowE
 		return workFlowRepository.save(workFlowExecution);
 	}

+	@Override
+	public List<WorkFlowResponseDTO> getWorkFlowsByProjectId(UUID projectId) {
+		List<ProjectResponseDTO> projects = projectId == null


Isn't it less predictable behavior? why not define projectId as @NotNull?
I'm not sure how users are created in the system, but what if the user has workflow in several projects?

yes then this controller will return all the workflows for this user, not limited to certain projects

so I think we should separate it into two service call, since the controller makes the decision on which service call to use, based on the input.

ok I will separate them

masayag · 2023-05-04T07:30:59Z

...service/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowServiceImpl.java

+				? projectService.findProjectsByUserName(SecurityUtils.getUsername())
+				: List.of(projectService.getProjectByIdAndUserName(projectId, SecurityUtils.getUsername()));
+		return projects.stream().flatMap(project -> workFlowRepository.findAllByProjectId(project.getId()).stream()
+				.filter(workFlowExecution -> workFlowExecution.getMainWorkFlowExecution() == null)


a lot is going on in this method, but in terms of readability, I find its readability being somewhat complex.
do you think it can be refactored a bit?

ok, I will move out the WorkFlowResponseDTO builder

masayag · 2023-05-04T07:32:23Z

workflow-service/src/test/java/com/redhat/parodos/project/repository/ProjectRepositoryTest.java

 public class ProjectRepositoryTest extends RepositoryTestBase {

+	@Autowired
+	private TestEntityManager entityManager;


entityManager is already defined in RepositoryTestBase as protected and being used by this class.

I will clean this

masayag · 2023-05-04T18:58:54Z

...service/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowServiceImpl.java

+	}
+
+	@Override
+	public List<WorkFlowResponseDTO> getWorkFlows() {


perhaps rename to getWorkFlowsForCurrentUser?
or to change the signature to accept username as a parameter and pass it from the caller (controller).

masayag · 2023-05-04T19:04:56Z

...low-service/src/main/java/com/redhat/parodos/workflow/execution/dto/WorkFlowResponseDTO.java

 public class WorkFlowResponseDTO {

 	private UUID workFlowExecutionId;

-	@JsonInclude(JsonInclude.Include.NON_EMPTY)
+	private String projectId;


pls change to UUID.

masayag · 2023-05-04T20:35:54Z

workflow-service/src/test/java/com/redhat/parodos/project/service/ProjectServiceImplTest.java


 		// then
-		assertEquals(exception.getMessage(), String.format("Project with id: %s not found", project.getId()));
+		assertNotNull(res);
+		assertEquals(res.getId().toString(), project.getId().toString());


toString() can be removed from both sides

openshift-ci · 2023-05-04T20:36:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: masayag, wKich

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [masayag]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

masayag · 2023-05-04T20:36:57Z

/lgtm

openshift-ci-robot added the jira/valid-reference label May 1, 2023

openshift-ci bot requested review from rgolangh and ydayagi May 1, 2023 19:48

RichardW98 requested review from anludke and wKich May 1, 2023 19:50

eloycoto suggested changes May 2, 2023

View reviewed changes

wKich approved these changes May 2, 2023

View reviewed changes

masayag reviewed May 2, 2023

View reviewed changes

gciavarrini reviewed May 2, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label May 3, 2023

RichardW98 force-pushed the filter-workflow-by-projectid branch 2 times, most recently from 7b81873 to 5ff2dcf Compare May 3, 2023 13:29

masayag reviewed May 3, 2023

View reviewed changes

RichardW98 force-pushed the filter-workflow-by-projectid branch 2 times, most recently from ebc3aa0 to 7f3e4b0 Compare May 3, 2023 13:51

openshift-merge-robot removed the needs-rebase label May 3, 2023

RichardW98 force-pushed the filter-workflow-by-projectid branch from 7f3e4b0 to 7c4d6ef Compare May 3, 2023 15:13

openshift-merge-robot added the needs-rebase label May 3, 2023

RichardW98 requested review from masayag, eloycoto and gciavarrini May 3, 2023 15:16

RichardW98 force-pushed the filter-workflow-by-projectid branch from 7c4d6ef to e7a540b Compare May 3, 2023 15:29

openshift-merge-robot removed the needs-rebase label May 3, 2023

RichardW98 force-pushed the filter-workflow-by-projectid branch from e7a540b to f17ccf1 Compare May 3, 2023 18:02

masayag reviewed May 4, 2023

View reviewed changes

RichardW98 force-pushed the filter-workflow-by-projectid branch from f17ccf1 to ff031ca Compare May 4, 2023 17:46

RichardW98 force-pushed the filter-workflow-by-projectid branch 2 times, most recently from c147156 to 9010b07 Compare May 4, 2023 18:03

RichardW98 requested review from masayag and pkliczewski May 4, 2023 18:03

masayag reviewed May 4, 2023

View reviewed changes

RichardW98 force-pushed the filter-workflow-by-projectid branch 2 times, most recently from 6d6d51e to f859af3 Compare May 4, 2023 19:09

get workflow by project id

6a74b24

RichardW98 force-pushed the filter-workflow-by-projectid branch from f859af3 to 6a74b24 Compare May 4, 2023 19:26

masayag approved these changes May 4, 2023

View reviewed changes

openshift-ci bot added the approved label May 4, 2023

openshift-ci bot assigned masayag May 4, 2023

openshift-ci bot added the lgtm label May 4, 2023

openshift-merge-robot merged commit 717ba82 into rhdhorchestrator:main May 4, 2023

RichardW98 deleted the filter-workflow-by-projectid branch May 8, 2023 15:10

		@@ -33,4 +35,8 @@ public interface ProjectService {

		List<ProjectResponseDTO> getProjects();

		List<ProjectResponseDTO> findProjectsByUserName(String userName);

FLPATH-313: get workflow by projectId #294

FLPATH-313: get workflow by projectId #294

Conversation

RichardW98 commented May 1, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented May 1, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented May 1, 2023 • edited by openshift-ci bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardW98 May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardW98 commented May 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented May 4, 2023

masayag commented May 4, 2023

RichardW98 commented May 1, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 1, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 1, 2023 •

edited by openshift-ci bot

Loading

RichardW98 May 2, 2023 •

edited

Loading