Fix dashboard URLs direct accessing issue and Modified URL patterns #732

lasanthaDLPDS · 2018-05-03T10:51:15Z

Purpose

The purpose of this PR is to fix TESTGRID dashboard URLs direct accessing issue. This resolves #425, resolves #731, resolves #730

Goals

In order to fix TESTGRID dashboard URLs direct accessing issue, check the existing of properties at the page loading time. If those are not available invoke d backend API and get required data to render pages. Further added bootstrap styling for pages and fixed issues which are related to displaying info on infra combination page and fixed formatting issues as well.

Approach

User stories

N/A

Release note

N/A

Documentation
N/A

Training
N/A

Certification
N/A

Marketing
N/A

Automation tests

Unit tests
N/A
Integration tests
N/A

Security checks

Followed secure coding standards in http://wso2.com/technical-reports/wso2-secure-engineering-guidelines? yes
Ran FindSecurityBugs plugin and verified report? yes
Confirmed that this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets? yes

Samples
N/A

Related PRs
N/A

Migrations (if applicable)
N/A

Test environment
JDK - 1.8
OS - Ubuntu
DB - MySQL
Browser - Chrome

Learning
N/A

In order to fix direct accessing issue, it is required to have these APIs. Since we are changing the URL patterns, changed web context root path parameter as well.

Modified URLs of the TESTGRID dashboard pages and fixed the direct accessing issue of those pages. Further added bootstrap styling for pages and fixed issues which are related to displaying info on infra combination page. Further fixed formatting issues as well.

harshanL · 2018-05-04T04:14:50Z

web/src/main/java/org/wso2/testgrid/web/api/ProductService.java

-                status.setId(product.getId());
-                status.setName(StringUtil.concatStrings(product.getName()));
+                status.setProductId(product.getId());
+                status.setProductName(StringUtil.concatStrings(product.getName()));


I think we don't need to use StringUtil.concatStrings here.

harshanL · 2018-05-04T04:19:18Z

web/src/main/java/org/wso2/testgrid/web/api/ProductService.java

@@ -138,6 +138,44 @@ public Response getAllProductStatuses() {
        return Response.status(Response.Status.OK).entity(list).build();
    }

+    /**
+     * This method returns the product that are currently in TestGrid.


This comment does not properly imply the purpose of the API. Shall we change it?

harshanL · 2018-05-04T04:20:00Z

web/src/main/java/org/wso2/testgrid/web/api/ProductService.java

+            if (productInstance.isPresent()) {
+                product = productInstance.get();
+                productStatus.setProductId(product.getId());
+                productStatus.setProductName(StringUtil.concatStrings(product.getName()));


I think we don't need to use StringUtil.concatStrings here.

azinneera · 2018-05-04T04:32:14Z

web/src/main/react-dashboard/src/components/TestRunView.js

+                                            <img
+                                                src={require('../success.png')}
+                                                style={{
+                                                    verticalAlign: "middle",


Shall we specify classes for these elements and write the styles in a separate css file? That way we will not need to duplicate the styles. We can do it as a separate effort btw.

+1 yes, we have to move this type of repeating styling into a separate CSS file. This fix resolves formatting issues and +1 for addressing this in a separate effort. Further, IMO we can use icons instead of these images. WDYT

+1 for moving to icons. Using images will slow down page loading time plus increase the size.

Let's raise a ticket and handle it elsewhere. We will have new set of statuses pretty soon as well.

Added requested changes from PR review and fixed some minor issues

kasunbg · 2018-05-04T06:12:56Z

web/src/main/java/org/wso2/testgrid/web/api/ProductService.java

+                productStatus.setProductStatus(testPlanUOW.getCurrentStatus(product).toString());
+            } else {
+                String msg = "Could not found a product for requested product name in the TestGrid.";
+                logger.error(msg);


How meaningful is this error log? Shall we improve the log message?

Fixed with 4be247e

kasunbg · 2018-05-04T06:14:18Z

web/src/main/java/org/wso2/testgrid/web/api/ProductService.java

+            return Response.status(Response.Status.OK).entity(productStatus).build();
+        } catch (TestGridDAOException e) {
+            String msg = "Error occurred while fetching the Product statuses ";
+            logger.error(msg, e);


Same as above. When running multiple products a production system, this error log won't provide enough data to debug an issue.

Fixed with 4be247e

kasunbg · 2018-05-04T06:18:38Z

web/src/main/react-dashboard/src/components/ProductStatusView.js

+                <td> <img src={require('../play.png')} width="36" height="36" data-tip="Execute job"  =>
+                { window.location = '/job/'+ product.productName +'/build' }} /> <ReactTooltip /></td>
+                <td ><img src={require('../configure.png')} width="36" height="36"  => { window.location =
+                    '/job/'+ product.productName +'/configure' }} data-tip="Configure job" style={{ cursor: 'pointer' }}/>


Is this URL correct? The /job/job-name/configure is coming from Jenkins webapp.

Fixed with 12488d2

kasunbg · 2018-05-04T06:19:02Z

web/src/main/react-dashboard/src/components/ProductStatusView.js

+                </Modal>
+            </div>
+        )
+    }
 }

 export default ProductStatusView;


Missing new line. Not your code though. :)
6D4E

Fixed with 5abab08

kasunbg · 2018-05-04T06:20:11Z

web/src/main/react-dashboard/src/components/SingleRecord.js

@@ -18,120 +18,120 @@

 import React, { Component } from 'react';
 import Moment from 'moment';
-import {FAIL,SUCCESS,ERROR,PENDING,RUNNING,INCOMPLETE,DID_NOT_RUN} from '../constants.js';
+import {SUCCESS,ERROR,PENDING,RUNNING,INCOMPLETE,DID_NOT_RUN} from '../constants.js';


Why remove FAIL?

Do we have some kind of js linting mechanism?

AFAIK we don't use js linting mechanism. IMO we can use https://eslint.org/. WDYT?

SingleRecord is a component and we have used it to return respective image for given status. In here we have used a switch statement and FAIL is treated as default. Hence removed the import of FAIL.

+1 for eslint.

kasunbg · 2018-05-04T06:20:47Z

web/src/main/react-dashboard/src/components/SingleRecord.js

    }
-  }
 }

 export default SingleRecord


missing new line.

Fixed with 5abab08

kasunbg · 2018-05-04T06:26:06Z

web/src/main/react-dashboard/src/components/TestRunView.js

+        this.state = {
+            testScenarioSummaries: [],
+            scenarioTestCaseEntries: [],
+            testSummaryLoadStatus: PENDING,


PENDING status means that the test is still running. Does it make sense to change the default status to DID_NOT_RUN which shows a gray-colored icon? When the actual data comes, we can populate with correct properties. wdyt?

+1 It is the better approach and let's raise a separate ticket and handle this as well.

kasunbg · 2018-05-04T06:28:20Z

web/src/main/react-dashboard/src/components/TestRunView.js

+        let currentInfra = {};
+        let currentUrl = window.location.href.split("/");
+        currentInfra.relatedProduct = currentUrl[currentUrl.length - 4];
+                      currentInfra.relatedDeplymentPattern = currentUrl[currentUrl.length - 3];

            


L63-L64 code logic is vulnerable for any change in URL contexts. It may fail silently. Is there a better way to handle this? Probably we can use some kind of regex or something similar to jax-rs @path annotation value.

This page will render when URL pattern matches to https://<host>:<port>/<product-ame>/<deployment-pattern-name>/test-plans/<testplan-id>.

kasunbg · 2018-05-04T06:30:07Z

web/src/main/react-dashboard/src/components/TestRunView.js

-      }
-      return response;
-  }
+    handleError(response) {


Shall we go with two-space indentation for JS files?
JS and HTML files tend to have lot of inner elements. Four-space indentation makes it a bit harder to read.

@sameeraw @lasanthaDLPDS @vidurananayakkara wdyt?

+1 Let's add two-space indentation for JS files.

Cool.. Can you change and re-send?

Fixed with 5c24430

kasunbg · 2018-05-04T06:31:05Z

web/src/main/react-dashboard/src/components/TestRunView.js

+                        case FAIL:
+                            return <Subheader style={{
+                                fontSize: '20px',
+                                backgroundColor: "#ffd6d3"


Is it advisable set css styling info within the js files like this? What's the best practice for better maintainability of the code?

AFAIK inline CSS is not a good practice. Let's raise a ticket and handle these kind of things. WDYT?

kasunbg · 2018-05-04T06:32:38Z

web/src/main/react-dashboard/src/components/TestRunView.js

+                                                            textAlign: "center",
+                                                            fontSize: "20px",
+                                                            wordWrap: "break-word",
+                                                            whiteSpace: "wrap"


hard-coded css within js files.

Shall we raise a separate ticket and handle this?

kasunbg · 2018-05-04T06:33:42Z

web/src/main/react-dashboard/src/components/TestRunView.js

+                                                                                :
+                                                                                <img width="36"
+                                                                                     height="36"
+                                                                                     src={require('../close.png')}/>}


@lasanthaDLPDS As you see, four-space indentation tend to make the codes right-aligned. :)
It's a bit harder to read as well.

Fixed with 5c24430

kasunbg · 2018-05-04T06:34:55Z

web/src/main/react-dashboard/src/containers/ScenarioContainer.js


 const ScenarioContainer = connect(
    mapStateToProps
-)(ScenarioView)
+)(ScenarioView);

 export default ScenarioContainer;


Missing new line.

Fixed with 5abab08

Some js files has missed new line at the end of the file. Hence added it for missed js files

lasanthaDLPDS added 2 commits May 3, 2018 15:22

Add required APIs to fix dashboard URLs direct accessing issue

b5586fa

In order to fix direct accessing issue, it is required to have these APIs. Since we are changing the URL patterns, changed web context root path parameter as well.

lasanthaDLPDS changed the title ~~Masterclone test~~ Fix dashboard URLs direct accessing issue and Modified URL patterns May 3, 2018

harshanL reviewed May 4, 2018

View reviewed changes

azinneera reviewed May 4, 2018

View reviewed changes

Fix minor issues

6d6e50a

Added requested changes from PR review and fixed some minor issues

harshanL approved these changes May 4, 2018

View reviewed changes

azinneera approved these changes May 4, 2018

View reviewed changes

harshanL added this to the 0.9.0-m24 milestone May 4, 2018

kasunbg approved these changes May 4, 2018

View reviewed changes

kasunbg reviewed May 4, 2018

View reviewed changes

lasanthaDLPDS added 4 commits May 4, 2018 12:43

Add a new line at the end of file

5abab08

Some js files has missed new line at the end of the file. Hence added it for missed js files

Modify response error messages and logging messages

4be247e

Add correct Jenkins URLs

12488d2

Use two spaces for indentation in all javascript files

5c24430

kasunbg approved these changes May 4, 2018

View reviewed changes

kasunbg merged commit 3ad18fb into wso2:master May 4, 2018

Fix dashboard URLs direct accessing issue and Modified URL patterns #732

Fix dashboard URLs direct accessing issue and Modified URL patterns #732

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!