Shubhendu Tripathi has posted comments on this change. Change subject: webadmin: UI for gluster volume snapshot creation ......................................................................
Patch Set 28: (22 comments) https://gerrit.ovirt.org/#/c/35082/28/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeSnapshotListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeSnapshotListModel.java: Line 177: getDeleteSnapshotCommand().setIsExecutionAllowed(allowDelete); Line 178: getDeleteAllSnapshotsCommand().setIsExecutionAllowed(allowDeleteAll); Line 179: getActivateSnapshotCommand().setIsExecutionAllowed(allowActivate); Line 180: getDeactivateSnapshotCommand().setIsExecutionAllowed(allowDeactivate); Line 181: getNewSnapshotCommand().setIsAvailable(allowCreateSnapshot); > I assume this command only needs to be disabled then please use setIsExecut Actually this gets disabled only when volume is down else enabled always for a volume. Line 182: getEditSnapshotScheduleCommand().setIsAvailable(allowEditSnapshotSchedule); Line 183: } Line 184: Line 185: Line 178: getDeleteAllSnapshotsCommand().setIsExecutionAllowed(allowDeleteAll); Line 179: getActivateSnapshotCommand().setIsExecutionAllowed(allowActivate); Line 180: getDeactivateSnapshotCommand().setIsExecutionAllowed(allowDeactivate); Line 181: getNewSnapshotCommand().setIsAvailable(allowCreateSnapshot); Line 182: getEditSnapshotScheduleCommand().setIsAvailable(allowEditSnapshotSchedule); > or is it that either edit or new can be (even) visible at a given point in Both New and Edit would be enabled if schedule exists. If a schedule does not exist for a volume only New would be enabled Line 183: } Line 184: Line 185: Line 186: @Override Line 583: getWindow > not used anywhere can you please remove it done Line 612: snapshotModel.setHashName("edit_volume_snapshot_schedule"); //$NON-NLS-1$ Line 613: snapshotModel.setTitle(constants.editVolumeSnapshotScheduleTitle()); Line 614: setWindow(snapshotModel); Line 615: Line 616: AsyncDataProvider.getInstance().getVolumeSnapshotSchedule(new AsyncQuery(this, new INewAsyncCallback() { > So, from the entry in asyncdateprovider I see that for some/any reason if t May be will try to do in another patch. Line 617: Line 618: @Override Line 619: public void onSuccess(Object model, Object returnValue) { Line 620: GlusterVolumeSnapshotSchedule schedule = (GlusterVolumeSnapshotSchedule) returnValue; Line 680: } Line 681: } Line 682: Line 683: private Date getExecutionTimeValue(GlusterVolumeSnapshotSchedule schedule) { Line 684: Date dt = new Date(); > I somewhere saw you using Time class for a similar requirement why not use The entity model is of type Date so returning a date Line 685: dt.setHours(schedule.getExecutionTime().getHours()); Line 686: dt.setMinutes(schedule.getExecutionTime().getMinutes()); Line 687: Line 688: return dt; Line 687: Line 688: return dt; Line 689: } Line 690: Line 691: private GlusterVolumeSnapshotScheduleRecurrence getRecurrenceType(org.ovirt.engine.core.common.businessentities.gluster.GlusterVolumeSnapshotScheduleRecurrence value) { > why not utilizing the same backend class? Back end class has enums UNKNOW and front end Minutely is mapped to INTERVAL based. So a mapper here. May be will try to do in another patch. Line 692: switch (value) { Line 693: case INTERVAL: Line 694: return GlusterVolumeSnapshotScheduleRecurrence.IntervalBased; Line 695: case HOURLY: https://gerrit.ovirt.org/#/c/35082/28/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeSnapshotModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeSnapshotModel.java: Line 38: private EntityModel<Date> executionTime; Line 39: private ListModel<List<DayOfWeek>> daysOfWeek; Line 40: private ListModel<String> daysOfMonth; Line 41: Line 42: public GlusterVolumeSnapshotModel(List<GlusterVolumeSnapshotScheduleRecurrence> recurrenceOptions, > why is it required to be passed here? Being used in init method to setItems() Line 43: boolean generalTabVisible, Line 44: boolean scheduleTabVisible) { Line 45: init(recurrenceOptions); Line 46: setAvailabilities(); Line 59: setEndByOptions(new ListModel<EndDateOptions>()); Line 60: setTimeZones(new ListModel<String>()); Line 61: setDaysOfMonth(new ListModel<String>()); Line 62: startAt = new EntityModel<>(); Line 63: startAt.setEntity(new Date()); > you could do this in previous statement itself as done Line 64: endDate = new EntityModel<>(); Line 65: endDate.setEntity(new Date()); Line 66: executionTime = new EntityModel<>(); Line 67: executionTime.setEntity(new Date()); Line 65: t > same as above done Line 63: startAt.setEntity(new Date()); Line 64: endDate = new EntityModel<>(); Line 65: endDate.setEntity(new Date()); Line 66: executionTime = new EntityModel<>(); Line 67: executionTime.setEntity(new Date()); > same as above done Line 68: initIntervals(); Line 69: initTimeZones(); Line 70: Line 71: recurrence.setItems(recurrenceOptions); Line 88: for (int nThMin = 1; mins < 55; nThMin++) { Line 89: mins = nThMin * 5; Line 90: intervals.add(String.valueOf(mins)); Line 91: } Line 92: getInterval().setItems(intervals, String.valueOf(5)); > what is this? I mean the 2nd arguement I remember it was for setting the interval. But yes its not required actually. Line 93: } Line 94: Line 95: private void initTimeZones() { Line 96: Set<String> timeZoneTypes = TimeZoneType.GENERAL_TIMEZONE.getTimeZoneList().keySet(); Line 96: Set<String> timeZoneTypes = TimeZoneType.GENERAL_TIMEZONE.getTimeZoneList().keySet(); Line 97: getTimeZones().setItems(timeZoneTypes); Line 98: } Line 99: Line 100: private void setAvailabilities() { > might not be required I assume the default availabilities are true ok. will try that Line 101: getClusterName().setIsAvailable(true); Line 102: getVolumeName().setIsAvailable(true); Line 103: getSnapshotName().setIsAvailable(true); Line 104: getDescription().setIsAvailable(true); Line 252: contains > why are you checking for position of ',' ? Last day cannot be selected with anything else so if anything else is selected with L it would come either as "L," or ",L" Line 260: setMessage(ConstantsManager.getInstance().getConstants().endDateBeforeStartDate()); Line 261: validEndDate = false; Line 262: } Line 263: Line 264: return getSnapshotName().getIsValid() && getDaysOfTheWeek().getIsValid() && getDaysOfMonth().getIsValid() > doubt : how does this work? We are setting message only in all the cases so it would be shown in the error label on the screen. Have tried this. Line 265: && validWeekDays && validMonthDays && validEndDate; Line 266: } Line 267: Line 268: public enum GlusterVolumeSnapshotScheduleRecurrence { Line 264: return getSnapshotName().getIsValid() && getDaysOfTheWeek().getIsValid() && getDaysOfMonth().getIsValid() Line 265: && validWeekDays && validMonthDays && validEndDate; Line 266: } Line 267: Line 268: public enum GlusterVolumeSnapshotScheduleRecurrence { > Doubt : Why not utilize the backend enum itself. Some value differences in UI and backend enums. May be will correct in a separate patch Line 269: None(constants.notScheduledOption()), Line 270: IntervalBased(constants.intervalBasedSnapshot()), Line 271: HourlySnapshot(constants.hourlySnapshot()), Line 272: DailySnapshot(constants.dailySnapshot()), https://gerrit.ovirt.org/#/c/35082/28/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/volumes/VolumeListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/volumes/VolumeListModel.java: Line 571: GlusterVolumeEntity volumeEntity = list.get(0); Line 572: return (volumeEntity.getStatus() == GlusterStatus.UP); Line 573: } else { Line 574: return false; Line 575: } > return ((list.size() == 0) && (list.get(0).getStatus() == GlusterStatus.UP) done Line 576: } Line 577: Line 578: private boolean isEditSnapshotScheduleAvailable(List<GlusterVolumeEntity> list) { Line 579: if (getSelectedItems().size() == 1) { Line 580: GlusterVolumeEntity volumeEntity = list.get(0); Line 581: return (volumeEntity.getStatus() == GlusterStatus.UP && volumeEntity.getSnapshotScheduled()); Line 582: } else { Line 583: return false; Line 584: } > same as above done Line 585: } Line 586: Line 587: private boolean isStopProfileAvailable(List<GlusterVolumeEntity> list) { Line 588: if (getSelectedItems().size() == 0) { https://gerrit.ovirt.org/#/c/35082/28/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java: Line 2600: @DefaultStringValue("Monthly") Line 2601: String monthlySnapshot(); Line 2602: Line 2603: @DefaultStringValue("AM") Line 2604: String snapshotScheduleMorning(); > May no longer be used right? yes will remove them Line 2605: Line 2606: @DefaultStringValue("PM") Line 2607: String snapshotScheduleEvening(); Line 2608: https://gerrit.ovirt.org/#/c/35082/28/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.java: Line 211: executionTimeEditor.setLabel(constants.executionTimeLabel()); Line 212: Line 213: criticalIntervalLabel.setText(constants.criticalSnapshotIntervalNote()); Line 214: criticalIntervalLabel.setVisible(false); Line 215: errorMsgLabel.setVisible(false); > please move the above 2 lines to a new method and have it called from const done Line 216: } Line 217: Line 218: @Override Line 219: public void edit(final GlusterVolumeSnapshotModel object) { Line 252: if (selectedItem == GlusterVolumeSnapshotScheduleRecurrence.DailySnapshot Line 253: || selectedItem == GlusterVolumeSnapshotScheduleRecurrence.WeeklySnapshot Line 254: || selectedItem == GlusterVolumeSnapshotScheduleRecurrence.MonthlySnapshot) { Line 255: executionTimeEditor.setVisible(true); Line 256: // executionTimeEditor.getContentWidget().showTimeOnly(); > please remove the comment done Line 257: timeZoneEditor.setVisible(true); Line 258: } else { Line 259: executionTimeEditor.setVisible(false); Line 260: timeZoneEditor.setVisible(false); Line 253: || selectedItem == GlusterVolumeSnapshotScheduleRecurrence.WeeklySnapshot Line 254: || selectedItem == GlusterVolumeSnapshotScheduleRecurrence.MonthlySnapshot) { Line 255: executionTimeEditor.setVisible(true); Line 256: // executionTimeEditor.getContentWidget().showTimeOnly(); Line 257: timeZoneEditor.setVisible(true); > please move model-view interactions to presenter done Line 258: } else { Line 259: executionTimeEditor.setVisible(false); Line 260: timeZoneEditor.setVisible(false); Line 261: } https://gerrit.ovirt.org/#/c/35082/28/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.ui.xml: Line 1: <?xml version="1.0" encoding="UTF-8"?> Line 2: <!DOCTYPE ui:UiBinder SYSTEM "http://dl.google.com/gwt/DTD/xhtml.ent";> Line 3: <ui:UiBinder xmlns:ui="urn:ui:com.google.gwt.uibinder" > can you please remove the unused imports done Line 4: xmlns:g="urn:import:com.google.gwt.user.client.ui" xmlns:d="urn:import:org.ovirt.engine.ui.common.widget.dialog" Line 5: xmlns:ge="urn:import:org.ovirt.engine.ui.common.widget.editor.generic" Line 6: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor" xmlns:w="urn:import:org.ovirt.engine.ui.common.widget" Line 7: xmlns:dp="urn:import:com.google.gwt.user.datepicker.client" xmlns:t="urn:import:org.ovirt.engine.ui.common.widget.dialog.tab"> -- To view, visit https://gerrit.ovirt.org/35082 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I823580ecb127e48e075c437668bfb19ff8ec4467 Gerrit-PatchSet: 28 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@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
