Alexander Wels has posted comments on this change. Change subject: webadmin: reports sso token ......................................................................
Patch Set 4: (9 comments) http://gerrit.ovirt.org/#/c/27542/4/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/GWTRPCCommunicationProvider.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/GWTRPCCommunicationProvider.java: Line 30: */ Line 31: @SuppressWarnings({ "unchecked", "rawtypes" }) Line 32: public class GWTRPCCommunicationProvider implements CommunicationProvider { Line 33: Line 34: private static final String SSO_TOKEN_HEADER = "X-OVIRT-SSO-TOKEN"; //$NON-NLS-1$ > Please see http://stackoverflow.com/questions/3561381/custom-http-headers-n Done Line 35: Line 36: /** Line 37: * GWT RPC service. Line 38: */ Line 58: @Inject Line 59: public GWTRPCCommunicationProvider(final GenericApiGWTServiceAsync asyncService, final EventBus eventBus) { Line 60: this.eventBus = eventBus; Line 61: this.service = asyncService; Line 62: ((ServiceDefTarget) this.service).setRpcRequestBuilder(new RpcRequestBuilder() { > This code modifies the state of injected singleton, GenericApiGWTServiceAsy Done Line 63: @Override Line 64: protected void doSetCallback(RequestBuilder rb, final RequestCallback callback) { Line 65: super.doSetCallback(rb, new RequestCallback() { Line 66: Line 61: this.service = asyncService; Line 62: ((ServiceDefTarget) this.service).setRpcRequestBuilder(new RpcRequestBuilder() { Line 63: @Override Line 64: protected void doSetCallback(RequestBuilder rb, final RequestCallback callback) { Line 65: super.doSetCallback(rb, new RequestCallback() { > In my opinion, just doing: Done Line 66: Line 67: @Override Line 68: public void onResponseReceived(Request request, Response response) { Line 69: String tokenValue = response.getHeader(SSO_TOKEN_HEADER); http://gerrit.ovirt.org/#/c/27542/4/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/SSOTokenChange.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/SSOTokenChange.java: Line 2: Line 3: import com.gwtplatform.dispatch.annotation.GenEvent; Line 4: Line 5: @GenEvent Line 6: public class SSOTokenChange { > Please add some brief Javadoc to explain the purpose of this event, for exa Done Line 7: String ssoToken; Line 3: import com.gwtplatform.dispatch.annotation.GenEvent; Line 4: Line 5: @GenEvent Line 6: public class SSOTokenChange { Line 7: String ssoToken; > Minor thing, I'd just use "token" as the class name makes it quite apparent Done http://gerrit.ovirt.org/#/c/27542/4/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/auth/SSOTokenData.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/auth/SSOTokenData.java: Line 1: package org.ovirt.engine.ui.common.auth; Line 2: Line 3: import com.google.gwt.core.client.JavaScriptObject; Line 4: Line 5: public final class SSOTokenData extends JavaScriptObject { > Please add a brief Javadoc comment, for example: Done Line 6: Line 7: protected SSOTokenData() { Line 8: } Line 9: Line 14: public native String getValue() /*-{ Line 15: return this.value; Line 16: }-*/; Line 17: Line 18: public String getSsoToken() { > I'd suggest to name this method just "getToken" as it's apparent that it re Done Line 19: return getValue(); Line 20: } http://gerrit.ovirt.org/#/c/27542/4/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/BaseApplicationInit.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/BaseApplicationInit.java: Line 172: getLoginModel().autoLogin(loggedUser); Line 173: } Line 174: }); Line 175: Line 176: SSOTokenChangeEvent.fire(eventBus, SSOTokenData.instance().getSsoToken()); > Shouldn't we first check if SSO token value is available? No, if the value is not available, the reports will simply be disabled, but we need to fire the event to make sure that the login process completes. Line 177: // Indicate that the user should be logged in automatically Line 178: user.setAutoLogin(true); Line 179: } Line 180: http://gerrit.ovirt.org/#/c/27542/4/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 180: new SSOTokenChangeHandler() { Line 181: Line 182: @Override Line 183: public void onSSOTokenChange(SSOTokenChangeEvent event) { Line 184: ssoToken = event.getSsoToken(); > Please consider using "ReportInit.this.ssoToken" for better readability. If it is null, I am setting it to "" so the login will continue will the reports turned off. Line 185: checkIfInitFinished(); Line 186: } Line 187: } Line 188: ); -- To view, visit http://gerrit.ovirt.org/27542 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib906a1cca87712fb4e31c68c5e8c70151976ce0f Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Greg Sheremeta <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[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
