Vered Volansky has posted comments on this change.
Change subject: core: add canDoAction to remove storage connection
......................................................................
Patch Set 7: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommand.java
Line 74: return
prepareFailureMessageForDomains(domainNamesForMessage) ;
Line 75: }
Line 76: else {
Line 77: String diskNamesForMessage =
prepareEntityNamesForMessage(diskNames);
Line 78: return
prepareFailureMessageForDomainsAndDisks(domainNamesForMessage,diskNamesForMessage);
Just a thought - did you consider just adding the two messages apart (disks &
domains) instead of a joint message? I know the one message is neater to the
user, but it involves more code to maintain for us, and there's nothing
logically wrong with two messages to the user.
Line 79: }
Line 80: }
Line 81: else if (!diskNames.isEmpty()) {
Line 82: String diskNamesForMessage =
prepareEntityNamesForMessage(diskNames);
Line 121: addCanDoActionMessage(String.format("$diskNames %1$s",
diskNames));
Line 122: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS_AND_DISKS);
Line 123: }
Line 124:
Line 125: protected String
createDomainNamesListFromStorageDomains(List<StorageDomain> domains) {
It'll be neater to extract the domains names to a list and use
StringUtils.join() here as well, or even prepareEntityNamesForMessage().
Line 126: // Build domain names list to display in the error
Line 127: StringBuilder domainNames = new StringBuilder();
Line 128: for (StorageDomain domain : domains) {
Line 129: domainNames.append(domain.getStorageName());
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 347: ACTION_TYPE_FAILED_STORAGE_CONNECTION_NOT_EXIST=Cannot ${action}
${type}. Storage connection doesn't exist.
Line 348: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_EMPTY=Cannot ${action}
${type}. Storage connection id is empty.
Line 349: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS=Cannot ${action}
${type}. Storage connection already exists.
Line 350:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE=Cannot
${action} ${type}. Storage connection parameters can be edited only for NFS,
Posix or local data domains in maintenance.
Line 351:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS=Cannot
${action} ${type}. Storage connection parameters are used by the following
storage domains : ${domainNames}.
I think it should still comment about the "more than one". This is also correct
for the following messages. Otherwise the first thought might be - so what?
Even if it's only for a brief moment, it's better to be as clear as possible.
Line 352:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS_AND_DISKS=Cannot
${action} ${type}. Storage connection parameters are used by the following
storage domains : ${domainNames} and disks: ${diskNames}.
Line 353: ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_DISKS=Cannot
${action} ${type}. Storage connection parameters are used by the following
disks : ${diskNames}.
Line 354:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_WRONG_PARAMETERS_FOR_STORAGE_TYPE=Cannot
${action} ${type}. Connection parameters are invalid for this storage type.
Line 355:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_CHANGE_STORAGE_TYPE=Cannot
${action} ${type}. Storage type cannot be edited.
....................................................
Commit Message
Line 6:
Line 7: core: add canDoAction to remove storage connection
Line 8:
Line 9: Add canDoAction validations before removing a storage connection
Line 10: that there are no storage domains nor lun disks using that connection.
This sentence is not clear. Suggestion:
Make sure there are no storage domains nor lun disks using a storage connection
before it is removed in RemoveStorageServerConnectionCommand.canDoaCtion().
Line 11: Also add corresponding unitests.
Line 12:
Line 13: Change-Id: Ib7a633b06195a7b8ca1cd4fc5023c05010a3161d
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java
Line 984:
Line 985: @DefaultStringValue("Cannot ${action} ${type}. Storage connection
parameters are used by the following storage domains : ${domainNames}.")
Line 986: String
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS();
Line 987:
Line 988: @DefaultStringValue("Cannot ${action} ${type}. Storage connection
parameters are used by the following storage domains : ${domainNames} and
disks: ${diskNames}.")
Same as VdcBllMessages
Line 989: String
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS_AND_DISKS();
Line 990:
Line 991: @DefaultStringValue("Cannot ${action} ${type}. Storage connection
parameters are used by the following disks : ${diskNames}.")
Line 992: String
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_DISKS();
--
To view, visit http://gerrit.ovirt.org/15269
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7a633b06195a7b8ca1cd4fc5023c05010a3161d
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
