Martin Mucha has posted comments on this change. Change subject: core: refactor&test for HostNetworkAttachmentsPersister ......................................................................
Patch Set 37: (6 comments) https://gerrit.ovirt.org/#/c/36067/37/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersisterTest.java File backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersisterTest.java: Line 150: } Line 151: Line 152: @Test Line 153: public void testPersistNetworkAttachmentsWhenCalledWithNewUserAttachments() throws Exception { Line 154: Mockito.when(networkAttachmentDao.getAllForHost(eq(hostId))).thenReturn(new ArrayList<NetworkAttachment>()); > Please statically import when Done Line 155: Line 156: NetworkAttachment userNetworkAttachment = createNetworkAttachment(clusterNetworkA); Line 157: Line 158: //when persisting new record user provided Id should be ignored and replaced. //TODO MM: is this really right? How about rejecting user input? Line 155: Line 156: NetworkAttachment userNetworkAttachment = createNetworkAttachment(clusterNetworkA); Line 157: Line 158: //when persisting new record user provided Id should be ignored and replaced. //TODO MM: is this really right? How about rejecting user input? Line 159: Guid useProvidedNetworkAttachmentId = userNetworkAttachment.getId(); > 1. The id passed by the user is not verified by the canDoActions, so it can I'm not sure if we understand each other. What I tried to say is, that if user is creating new record and provides HIS own IDs to be used for that record, his request should be rejected and not fixed (by overriding IDs). The rejection could occur in canDoAction, sure. Now, what do we want to have? Validation in canDoAction or simple overwriting here (with no todo)? And I do think users value shouldn't be used as a ID, but I guess this is not what's you're asking for. Line 160: Line 161: //user sends some possibly nonsensical value, but code will override it for him //TODO MM: is this really right? Line 162: userNetworkAttachment.setNicId(Guid.newGuid()); Line 163: Line 157: Line 158: //when persisting new record user provided Id should be ignored and replaced. //TODO MM: is this really right? How about rejecting user input? Line 159: Guid useProvidedNetworkAttachmentId = userNetworkAttachment.getId(); Line 160: Line 161: //user sends some possibly nonsensical value, but code will override it for him //TODO MM: is this really right? > As I wrote on the previous patch, I don't see any reason overriding it. see above. Line 162: userNetworkAttachment.setNicId(Guid.newGuid()); Line 163: Line 164: //user attachments references network, which is not assigned to NIC. Line 165: createPersister(Arrays.asList(userNetworkAttachment)).persistNetworkAttachments(); Line 182: Line 183: @Test Line 184: public void testPersistNetworkAttachmentsWhenCalledWithAlreadyExistingAttachmentItWillUpdated() throws Exception { Line 185: NetworkAttachment userNetworkAttachment = createNetworkAttachment(clusterNetworkA); Line 186: Guid useProvidedNetworkAttachmentId = userNetworkAttachment.getId(); > s/useProvidedNetworkAttachmentId/userProvidedNetworkAttachmentId Done Line 187: Line 188: Mockito.when(networkAttachmentDao.getAllForHost(eq(hostId))) Line 189: .thenReturn(new ArrayList<>(Arrays.asList(userNetworkAttachment))); Line 190: Line 188: Mockito.when(networkAttachmentDao.getAllForHost(eq(hostId))) Line 189: .thenReturn(new ArrayList<>(Arrays.asList(userNetworkAttachment))); Line 190: Line 191: Line 192: //user attachments references network, which is not assigned to NIC. > Please add space after the // - relevant to all the comments in the class. all fixed. Done. Line 193: createPersister(Arrays.asList(userNetworkAttachment)).persistNetworkAttachments(); Line 194: Line 195: verify(networkAttachmentDao).getAllForHost(any(Guid.class)); Line 196: https://gerrit.ovirt.org/#/c/36067/37/frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties File frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties: Line 487: NETWORK_INTERFACE_ALREADY_HAVE_NETWORK=The Network Interface is already attached to a Logical Network. Line 488: NETWORK_ALREAY_ATTACH_TO_INTERFACE=Logical Network is already attached to a Network Interface. Line 489: NETWORK_NOT_EXISTS=The specified Logical Network doesn't exist. Line 490: ACTION_TYPE_FAILED_NETWORK_QOS_NOT_EXISTS=The specified Network QoS doesn't exist. Line 491: ACTION_TYPE_FAILED_NETWORK_ATTACHMENT_CONTAINS_DUPLICATES=Provided NetworkAttachments contains duplicates. > This error is not relevant for user-portal. You don't need to add it to thi Done Line 492: ACTION_TYPE_FAILED_VNIC_PROFILE_NOT_EXISTS=Cannot ${action} ${type}. The specified VM network interface profile doesn't exist. Line 493: ACTION_TYPE_FAILED_VNIC_PROFILE_NAME_IN_USE=Cannot ${action} ${type}. The VM network interface profile's name is already used by an existing profile for the same network.\n-Please choose a different name. Line 494: ACTION_TYPE_FAILED_VNIC_PROFILE_IN_USE=Cannot ${action} ${type}. Several ${entities} (${ENTITIES_USING_VNIC_PROFILE_COUNTER}) are using this VM network interface profile:\n${ENTITIES_USING_VNIC_PROFILE}\n - Please remove it from all ${entities} that are using it and try again. Line 495: ACTION_TYPE_FAILED_CANNOT_CHANGE_VNIC_PROFILE_NETWORK=Cannot ${action} ${type}. VM network interface profile's network cannot be changed. -- To view, visit https://gerrit.ovirt.org/36067 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib39c3678a29a7bd8728d32b55490f8500a77d9d6 Gerrit-PatchSet: 37 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
