Alona Kaplan has posted comments on this change. Change subject: engine, webadmin: Webadmin read reports.xml from remote reports app ......................................................................
Patch Set 12: (8 comments) http://gerrit.ovirt.org/#/c/29723/12/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/ReportInit.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/ReportInit.java: Line 56: // the re-init can happen after logout/login. Line 57: // As this class has it's state, it needs to be inited again Line 58: initState(); Line 59: setReportBaseUrl((String) AsyncDataProvider.getConfigValuePreConverted(ConfigurationValues.RedirectServletReportsPage)); Line 60: parseReportsXML(); 1. please format 2. I think the following code should be here (and not where it is now)- if (!reportBaseUrl.trim().isEmpty()) { parseReportsXML(); } else { setXmlInitialized(); } Is there any meaning parsing the xml if the url is empty? Line 61: } Line 62: Line 63: private void initState() { Line 64: reportsEnabled = false; Line 93: return new RequestBuilder(RequestBuilder.GET, url); Line 94: } Line 95: Line 96: private void scheduleCheckStatus() { Line 97: if (scheduledStatusCheckInProgress || reportsWebappDeployed) { I see you"ve moved 'reportsWebappDeployed' but what about 'retryCount > MAX_RETRY_COUNTS'? Line 98: return; Line 99: } Line 100: scheduledStatusCheckInProgress = true; Line 101: Scheduler.get().scheduleFixedDelay( Line 143: + BaseContextPathData.getInstance().getRelativePath() Line 144: + "services/reports-xml").sendRequest(null, new RequestCallback() { //$NON-NLS-1$ Line 145: @Override Line 146: public void onError(Request request, Throwable exception) { Line 147: if (!reportBaseUrl.trim().isEmpty()) { please format Line 148: scheduleCheckStatus(); Line 149: } Line 150: setXmlInitialized(); Line 151: } Line 145: @Override Line 146: public void onError(Request request, Throwable exception) { Line 147: if (!reportBaseUrl.trim().isEmpty()) { Line 148: scheduleCheckStatus(); Line 149: } please format Line 150: setXmlInitialized(); Line 151: } Line 152: Line 153: @Override Line 159: scheduleCheckStatus(); Line 160: } Line 161: break; Line 162: case Response.SC_OK: Line 163: if (ReportParser.getInstance().parseReport(response.getText())) { Shouldn't the try-catch surround just this if? Line 164: resourceMap = ReportParser.getInstance().getResourceMap(); Line 165: dashboardMap = ReportParser.getInstance().getDashboardMap(); Line 166: isCommunityEdition = ReportParser.getInstance().isCommunityEdition(); Line 167: } http://gerrit.ovirt.org/#/c/29723/12/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/tab/MainTabReportsPresenter.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/tab/MainTabReportsPresenter.java: Line 70: Line 71: private void checkUpdateReportsPanel(final String url, Line 72: final Map<String, List<String>> params) { Line 73: try { Line 74: ReportInit.constructRequestBuilder(buildStatusUrl()).sendRequest(null, new RequestCallback() { If you"ll change constructServiceRequestBuilder as I suggested you can use here- ReportInit.constructServiceRequestBuilder(ReportInit.ReportStatusService). buildStatusUrl() method can be removed. Line 75: @Override Line 76: public void onError(Request request, Throwable exception) { Line 77: errorPopupManager.show(applicationConstants.reportsWebAppNotDeployedMsg()); Line 78: } http://gerrit.ovirt.org/#/c/29723/12/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/system/ApplicationInit.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/system/ApplicationInit.java: Line 100: protected void onLogin(final LoginModel loginModel) { Line 101: // Initialize reports Line 102: ReportInit.getInstance().init(); Line 103: Line 104: // Perform login only after reports have been initialized Please update the comment. Line 105: ReportInit.getInstance().getReportsInitEvent().addListener(new IEventListener() { Line 106: @Override Line 107: public void eventRaised(Event ev, Object sender, EventArgs args) { Line 108: updateAvailability(); Line 104: // Perform login only after reports have been initialized Line 105: ReportInit.getInstance().getReportsInitEvent().addListener(new IEventListener() { Line 106: @Override Line 107: public void eventRaised(Event ev, Object sender, EventArgs args) { Line 108: updateAvailability(); updateAvialability() calls -> updateAvailability(SystemTreeItemType.System, null); Since the reports init is no longer blocking the ui there is no guarantee the system tree selected item is the 'System'. Calling updateAvialability() will update all the tabs (not just the reports) as if the system is selected. In my opinion, you should extract a method in CommonModel to update just the reports availability and call in from here. Line 109: } Line 110: }); Line 111: Line 112: performLogin(loginModel); -- To view, visit http://gerrit.ovirt.org/29723 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I76db7ab889f21de083bb3c8276e8abb77b68fdb3 Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Shirly Radco <[email protected]> Gerrit-Reviewer: Yaniv Dary <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
