Francesco Romani has posted comments on this change. Change subject: userportal, webadmin: VM console key management ......................................................................
Patch Set 20: (9 comments) Thanks for the review! https://gerrit.ovirt.org/#/c/35810/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/PublicKeyPopupWidget.ui.xml File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/PublicKeyPopupWidget.ui.xml: Line 3: <ui:UiBinder xmlns:ui="urn:ui:com.google.gwt.uibinder" xmlns:g="urn:import:com.google.gwt.user.client.ui" Line 4: xmlns:ge="urn:import:org.ovirt.engine.ui.common.widget.editor.generic"> Line 5: Line 6: <ui:style type="org.ovirt.engine.ui.common.widget.uicommon.popup.vm.PublicKeyPopupWidget.Style"> Line 7: .content { > why do you have all of this empty styles here? Id say you could just delete Done Line 8: Line 9: } Line 10: Line 11: .publicKeyEditor { Line 20: Line 21: } Line 22: </ui:style> Line 23: Line 24: <g:VerticalPanel verticalAlignment="ALIGN_MIDDLE" addStyleNames="{style.content}"> > please use FlowPanel instead - VerticalPanel renders as <table> and we are Done Line 25: <ge:StringEntityModelTextAreaEditor ui:field="publicKeyEditor" addStyleNames="{style.publicKeyEditor}" /> Line 26: </g:VerticalPanel> Line 27: https://gerrit.ovirt.org/#/c/35810/20/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/PublicKeyModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/PublicKeyModel.java: Line 1: package org.ovirt.engine.ui.uicommonweb.models; Line 2: Line 3: public class PublicKeyModel extends Model { Line 4: Line 5: EntityModel<String> textInput; > private EntityModel... Done Line 6: Line 7: public EntityModel<String> getTextInput() { Line 8: return textInput; Line 9: } https://gerrit.ovirt.org/#/c/35810/20/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/userportal/UserPortalListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/userportal/UserPortalListModel.java: Line 545: model.setHelpTag(HelpTag.edit_public_key); Line 546: Line 547: model.getCommands().add(UICommand.createDefaultOkUiCommand("OnSetConsoleKey", this)); //$NON-NLS-1$ Line 548: model.getCommands().add(UICommand.createCancelUiCommand("Cancel", this)); //$NON-NLS-1$ Line 549: > here you should call "model.startProgress(null)" (to show the loading indic Done Line 550: AsyncDataProvider.getInstance().getUserProfile(new AsyncQuery(model, Line 551: new INewAsyncCallback() { Line 552: @Override Line 553: public void onSuccess(Object target, Object returnValue) { Line 550: AsyncDataProvider.getInstance().getUserProfile(new AsyncQuery(model, Line 551: new INewAsyncCallback() { Line 552: @Override Line 553: public void onSuccess(Object target, Object returnValue) { Line 554: UserProfile profile = ((VdcQueryReturnValue) returnValue).getReturnValue(); > I'd say this is NPE vulnerable since the profile could be null Added if() to check against nullness Line 555: model.getTextInput().setEntity(profile.getSshPublicKey()); Line 556: } Line 557: })); Line 558: } Line 551: new INewAsyncCallback() { Line 552: @Override Line 553: public void onSuccess(Object target, Object returnValue) { Line 554: UserProfile profile = ((VdcQueryReturnValue) returnValue).getReturnValue(); Line 555: model.getTextInput().setEntity(profile.getSshPublicKey()); > and here "model.stopProgress()" Done Line 556: } Line 557: })); Line 558: } Line 559: Line 559: Line 560: private void onSetConsoleKey() { Line 561: Line 562: final PublicKeyModel model = (PublicKeyModel) getWindow(); Line 563: > here should be a check if the something is not going on in the model on bac Done Line 564: final String publicKey = model.getTextInput().getEntity(); Line 565: Line 566: model.startProgress(null); Line 567: Line 569: new INewAsyncCallback() { Line 570: @Override Line 571: public void onSuccess(Object target, Object returnValue) { Line 572: UserProfile profile = ((VdcQueryReturnValue) returnValue).getReturnValue(); Line 573: VdcActionType action = (profile != null) ?VdcActionType.UpdateUserProfile :VdcActionType.AddUserProfile; > just a nit - please add space between "?" and "VdcAc..". The same with ":" Done Line 574: Line 575: Frontend.getInstance().runAction(action, new UserProfileParameters(publicKey), Line 576: new IFrontendActionAsyncCallback() { Line 577: @Override https://gerrit.ovirt.org/#/c/35810/20/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java: Line 1751: } Line 1752: }, model); Line 1753: } Line 1754: Line 1755: private void editConsoleKey() { > well, this is copy / pasted from UserPortalListModel. Please move this logi Ops. Will refactor and remove/minimize (at least!) duplication. Line 1756: Line 1757: final PublicKeyModel model = new PublicKeyModel(); Line 1758: Line 1759: setWindow(model); -- To view, visit https://gerrit.ovirt.org/35810 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id2364cafc687ba6dee2504322234067ff98dc00c Gerrit-PatchSet: 20 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vitor de Lima <vdel...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
