Omer Frenkel has posted comments on this change. Change subject: core, restapi : Add /tags sub-collection for Template resource ......................................................................
Patch Set 4: (8 comments) http://gerrit.ovirt.org/#/c/24577/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachTemplatesToTagCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachTemplatesToTagCommand.java: Line 14: } Line 15: Line 16: @Override Line 17: protected void executeCommand() { Line 18: if (getTagId() != null) { although i see this is how other tags commands are written, they are pretty old, and this is not the standard way to write commands, basically each command should have a 'CanDoAction' method that perform all needed checks (here it looks like getTagId() should not be null) and fail the command with a proper translated error. then the execute phase should be clean of checks. what do you think? should be make new commands in the standard way? or should we stick with this not clear tags mess? Line 19: for (Guid templateGuid : getTemplatesList()) { Line 20: VmTemplate template = DbFacade.getInstance().getVmTemplateDao().get(templateGuid); Line 21: if (DbFacade.getInstance().getTagDao().getTagTemplateByTagIdAndByTemplateId(getTagId(), templateGuid) == null) { Line 22: if (template != null) { Line 22: if (template != null) { Line 23: appendCustomValue("TemplatesNames", template.getName(), ", "); Line 24: } Line 25: TagsTemplateMap map = new TagsTemplateMap(getTagId(), templateGuid); Line 26: DbFacade.getInstance().getTagDao().attachTemplateToTag(map); why would we do this if template is null? Line 27: noActionDone = false; Line 28: } else { Line 29: if (template != null) { Line 30: appendCustomValue("TemplatesNamesExists", template.getName(), ", "); http://gerrit.ovirt.org/#/c/24577/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachTemplateFromTagCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachTemplateFromTagCommand.java: Line 14: Line 15: @Override Line 16: protected void executeCommand() { Line 17: for (Guid templateGuid : getTemplatesList()) { Line 18: if (getTagId() != null it would be better to check getTagId() != null only once Line 19: && DbFacade.getInstance().getTagDao().getTagTemplateByTagIdAndByTemplateId(getTagId(), templateGuid) != null) { Line 20: VmTemplate template = DbFacade.getInstance().getVmTemplateDao().get(templateGuid); Line 21: if (template != null) { Line 22: appendCustomValue("TemplatesNames", template.getName(), ", "); Line 21: if (template != null) { Line 22: appendCustomValue("TemplatesNames", template.getName(), ", "); Line 23: } Line 24: DbFacade.getInstance().getTagDao().detachTemplateFromTag(getTagId(), templateGuid); Line 25: setSucceeded(true); i guess this should be out of the for loop, no point setting the succeeded over and over to true, also before all the actions are done. Line 26: } Line 27: } Line 28: } Line 29: http://gerrit.ovirt.org/#/c/24577/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/TemplatesTagMapBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/TemplatesTagMapBase.java: Line 3: import org.ovirt.engine.core.common.action.AttachEntityToTagParameters; Line 4: import org.ovirt.engine.core.compat.Guid; Line 5: Line 6: public abstract class TemplatesTagMapBase<T extends AttachEntityToTagParameters> extends TagsCommandBase<T> { Line 7: protected java.util.ArrayList<Guid> getTemplatesList() { please import java.util instead of explicitly writing here Line 8: return getParameters().getEntitiesId(); Line 9: } Line 10: Line 11: public TemplatesTagMapBase(T parameters) { http://gerrit.ovirt.org/#/c/24577/4/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java: Line 514: USER_ATTACH_TAG_TO_TEMPLATE(456), Line 515: USER_ATTACH_TAG_TO_TEMPLATE_FAILED(457), Line 516: USER_DETACH_TEMPLATE_FROM_TAG(458), Line 517: USER_DETACH_TEMPLATE_FROM_TAG_FAILED(459), Line 518: USER_ATTACH_TAG_TO_TEMPLATE_EXISTS(460), missing severity set in the AuditLogDirector Line 519: USER_MOVE_TAG(555), Line 520: USER_MOVE_TAG_FAILED(556), Line 521: USER_LOGGED_IN_VM(456), Line 522: USER_LOGGED_OUT_VM(457), http://gerrit.ovirt.org/#/c/24577/4/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetTagsByTemplateIdParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetTagsByTemplateIdParameters.java: Line 6: public GetTagsByTemplateIdParameters(String templateId) { Line 7: _templateId = templateId; Line 8: } Line 9: Line 10: private String _templateId; please remove the underscore from the member name, this is not java style Line 11: Line 12: public String getTemplateId() { Line 13: return _templateId; Line 14: } http://gerrit.ovirt.org/#/c/24577/4/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/TagDAO.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/TagDAO.java: Line 103: * @param ids Line 104: * the Template ids Line 105: * @return the list of tags Line 106: */ Line 107: List<Tags> getAllForTemplate(String ids); please add DAO tests for new methods Line 108: Line 109: /** Line 110: * Retrieves the list of VM tags for the given tag ids. Line 111: * -- To view, visit http://gerrit.ovirt.org/24577 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I32e2d449f7154c50bc7511e143ed82c995a3fa4e Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[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
