Thanks for your review Scott. There really are a lot of issues with this commit...
>From a higher level perspective, why is this implemented with so much code and >so many new files? I think all that is needed to implement this functionality >is a service definition, simple-method service implementation, and a couple of >SECA rules. -David On Dec 26, 2009, at 7:35 PM, Scott Gray wrote: > Hi Jacques, > > I haven't reviewed the actual logic but a few comments inline. > > Regards > Scott > > HotWax Media > http://www.hotwaxmedia.com > > On 27/12/2009, at 12:21 AM, jler...@apache.org wrote: > >> Author: jleroux >> Date: Sat Dec 26 11:21:21 2009 >> New Revision: 893961 >> >> URL: http://svn.apache.org/viewvc?rev=893961&view=rev >> Log: >> * Introduces a new createUpdatePartyRelationshipAndRoles service >> * Cleans imports of PartyHelper.java >> * A new getActivePartyRelationships method in new PartyRelationshipHelper >> class >> * A new checkPartyType method in new PartyTypeHelper class >> >> Added: >> >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipHelper.java >> (with props) >> >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyTypeHelper.java >> (with props) >> Modified: >> ofbiz/trunk/applications/party/servicedef/services.xml >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyHelper.java >> >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipServices.java >> >> Modified: ofbiz/trunk/applications/party/servicedef/services.xml >> URL: >> http://svn.apache.org/viewvc/ofbiz/trunk/applications/party/servicedef/services.xml?rev=893961&r1=893960&r2=893961&view=diff >> ============================================================================== >> --- ofbiz/trunk/applications/party/servicedef/services.xml (original) >> +++ ofbiz/trunk/applications/party/servicedef/services.xml Sat Dec 26 >> 11:21:21 2009 >> @@ -351,6 +351,22 @@ >> <auto-attributes include="nonpk" mode="IN" optional="true"/> >> <override name="partyRelationshipName" optional="false"/> >> </service> >> + >> + <service name="createUpdatePartyRelationshipAndRoles" engine="java" >> default-entity-name="PartyRelationship" >> + location="org.ofbiz.party.party.PartyRelationshipServices" >> invoke="createUpdatePartyRelationshipAndRoles" auth="true"> >> + <description> >> + Create or update both parties roles and parties relationship, >> partyRelationshipTypeId being mandatory. >> + The relationship is considered from one side or another >> (partyId is checked internally against partyIdFrom) >> + If a type of parties relationship exists PartyIdTo or >> PartyIdFrom are updated. >> + The history is maintained, allowing to track changes. >> + </description> >> + <auto-attributes include="pk" mode="IN" optional="false"/> >> + <auto-attributes include="nonpk" mode="IN" optional="true"/> >> + <attribute name="partyId" type="String" mode="IN" optional="false"/> >> + <override name="partyId" optional="false"/> > > This partyId override seems redundant > >> + <override name="fromDate" optional="true"/> >> + </service> >> + >> >> <service name="createPartyRelationshipContactAccount" engine="simple" >> >> location="component://party/script/org/ofbiz/party/party/PartyServices.xml" >> invoke="createPartyRelationshipContactAccount" auth="true"> >> >> Modified: >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyHelper.java >> URL: >> http://svn.apache.org/viewvc/ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyHelper.java?rev=893961&r1=893960&r2=893961&view=diff >> ============================================================================== >> --- >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyHelper.java >> (original) >> +++ >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyHelper.java >> Sat Dec 26 11:21:21 2009 >> @@ -19,20 +19,13 @@ >> >> package org.ofbiz.party.party; >> >> -import java.util.Locale; >> -import java.util.Map; >> - >> import org.ofbiz.base.util.Debug; >> import org.ofbiz.base.util.UtilFormatOut; >> import org.ofbiz.base.util.UtilMisc; >> -import org.ofbiz.base.util.UtilProperties; >> import org.ofbiz.entity.Delegator; >> import org.ofbiz.entity.GenericEntityException; >> import org.ofbiz.entity.GenericValue; >> import org.ofbiz.entity.model.ModelEntity; >> -import org.ofbiz.security.Security; >> -import org.ofbiz.service.ModelService; >> -import org.ofbiz.service.ServiceUtil; >> >> /** >> * PartyHelper >> >> Added: >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipHelper.java >> URL: >> http://svn.apache.org/viewvc/ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipHelper.java?rev=893961&view=auto >> ============================================================================== >> --- >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipHelper.java >> (added) >> +++ >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipHelper.java >> Sat Dec 26 11:21:21 2009 >> @@ -0,0 +1,85 @@ >> +/******************************************************************************* >> + * Licensed to the Apache Software Foundation (ASF) under one >> + * or more contributor license agreements. See the NOTICE file >> + * distributed with this work for additional information >> + * regarding copyright ownership. The ASF licenses this file >> + * to you under the Apache License, Version 2.0 (the >> + * "License"); you may not use this file except in compliance >> + * with the License. You may obtain a copy of the License at >> + * >> + * http://www.apache.org/licenses/LICENSE-2.0 >> + * >> + * Unless required by applicable law or agreed to in writing, >> + * software distributed under the License is distributed on an >> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY >> + * KIND, either express or implied. See the License for the >> + * specific language governing permissions and limitations >> + * under the License. >> + >> *******************************************************************************/ >> + >> +package org.ofbiz.party.party; >> + >> +import java.sql.Timestamp; >> +import java.util.List; >> +import java.util.Map; >> + >> +import javolution.util.FastList; >> + >> +import org.ofbiz.base.util.Debug; >> +import org.ofbiz.base.util.UtilDateTime; >> +import org.ofbiz.base.util.UtilMisc; >> +import org.ofbiz.base.util.UtilValidate; >> +import org.ofbiz.entity.Delegator; >> +import org.ofbiz.entity.GenericEntityException; >> +import org.ofbiz.entity.GenericValue; >> +import org.ofbiz.entity.condition.EntityCondition; >> +import org.ofbiz.entity.condition.EntityOperator; >> + >> +/** >> + * PartyRelationshipHelper >> + */ >> +public class PartyRelationshipHelper { >> + >> + public static final String module = >> PartyRelationshipHelper.class.getName(); >> + >> + /** Return A List of the active Party Relationships (ie with valid from >> and thru dates) >> + *...@param delegator needed Delegator >> + *...@param partyRelationshipValues Map containing the input parameters >> (primaries keys + partyRelationshipTypeId) >> + *...@return List of the active Party Relationships >> + */ >> + public static List<GenericValue> getActivePartyRelationships(Delegator >> delegator, Map<String, ?> partyRelationshipValues) { >> + String partyIdFrom = (String) >> partyRelationshipValues.get("partyIdFrom") ; >> + String partyIdTo = (String) >> partyRelationshipValues.get("partyIdTo") ; >> + String roleTypeIdFrom = (String) >> partyRelationshipValues.get("roleTypeIdFrom") ; >> + String roleTypeIdTo = (String) >> partyRelationshipValues.get("roleTypeIdTo") ; >> + String partyRelationshipTypeId = (String) >> partyRelationshipValues.get("partyRelationshipTypeId") ; >> + Timestamp fromDate = UtilDateTime.nowTimestamp(); >> + >> + List<EntityCondition> condList = FastList.newInstance(); >> + condList.add(EntityCondition.makeCondition("partyIdFrom", >> partyIdFrom)); >> + condList.add(EntityCondition.makeCondition("partyIdTo", partyIdTo)); >> + condList.add(EntityCondition.makeCondition("roleTypeIdFrom", >> roleTypeIdFrom)); >> + condList.add(EntityCondition.makeCondition("roleTypeIdTo", >> roleTypeIdTo)); >> + >> condList.add(EntityCondition.makeCondition("partyRelationshipTypeId", >> partyRelationshipTypeId)); >> + condList.add(EntityCondition.makeCondition("fromDate", >> EntityOperator.LESS_THAN_EQUAL_TO, fromDate)); >> + EntityCondition thruCond = >> EntityCondition.makeCondition(UtilMisc.toList( >> + EntityCondition.makeCondition("thruDate", null), >> + EntityCondition.makeCondition("thruDate", >> EntityOperator.GREATER_THAN, fromDate)), >> + EntityOperator.OR); >> + condList.add(thruCond); > > EntityUtil has some methods for filtering by fromDate/thruDate which makes > this a bit easier > >> + EntityCondition condition = EntityCondition.makeCondition(condList); >> + >> + List<GenericValue> partyRelationships = null; >> + try { >> + partyRelationships = delegator.findList("PartyRelationship", >> condition, null, null, null, false); >> + } catch (GenericEntityException e) { >> + Debug.logError(e, "Problem finding PartyRelationships. ", >> module); >> + return null; >> + } >> + if (UtilValidate.isNotEmpty(partyRelationships)) { >> + return partyRelationships; >> + } else { >> + return null; >> + } >> + } >> +} >> >> Propchange: >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipHelper.java >> ------------------------------------------------------------------------------ >> svn:eol-style = native >> >> Propchange: >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipHelper.java >> ------------------------------------------------------------------------------ >> svn:keywords = "Date Rev Author URL Id" >> >> Propchange: >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipHelper.java >> ------------------------------------------------------------------------------ >> svn:mime-type = text/plain >> >> Modified: >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipServices.java >> URL: >> http://svn.apache.org/viewvc/ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipServices.java?rev=893961&r1=893960&r2=893961&view=diff >> ============================================================================== >> --- >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipServices.java >> (original) >> +++ >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRelationshipServices.java >> Sat Dec 26 11:21:21 2009 >> @@ -1,15 +1,15 @@ >> /******************************************************************************* >> - * Licensed to the Apache Software Foundation (ASF) under one >> + * Licensed partyIdTo the Apache Software Foundation (ASF) under one >> * or more contributor license agreements. See the NOTICE file >> * distributed with this work for additional information >> * regarding copyright ownership. The ASF licenses this file >> - * to you under the Apache License, Version 2.0 (the >> + * partyIdTo you under the Apache License, Version 2.0 (the >> * "License"); you may not use this file except in compliance >> * with the License. You may obtain a copy of the License at >> * >> * http://www.apache.org/licenses/LICENSE-2.0 >> * >> - * Unless required by applicable law or agreed to in writing, >> + * Unless required by applicable law or agreed partyIdTo in writing, >> * software distributed under the License is distributed on an >> * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY >> * KIND, either express or implied. See the License for the > > Unintended changes in the license? > >> @@ -19,20 +19,27 @@ >> >> package org.ofbiz.party.party; >> >> +import java.util.List; >> import java.util.Map; >> >> import javolution.util.FastMap; >> >> import org.ofbiz.base.util.Debug; >> +import org.ofbiz.base.util.UtilDateTime; >> import org.ofbiz.base.util.UtilMisc; >> +import org.ofbiz.base.util.UtilValidate; >> import org.ofbiz.entity.Delegator; >> import org.ofbiz.entity.GenericEntityException; >> import org.ofbiz.entity.GenericValue; >> +import org.ofbiz.entity.util.EntityUtil; >> import org.ofbiz.security.Security; >> import org.ofbiz.service.DispatchContext; >> +import org.ofbiz.service.GenericServiceException; >> +import org.ofbiz.service.LocalDispatcher; >> import org.ofbiz.service.ModelService; >> import org.ofbiz.service.ServiceUtil; >> >> + >> /** >> * Services for Party Relationship maintenance >> */ >> @@ -52,7 +59,7 @@ >> Security security = ctx.getSecurity(); >> GenericValue userLogin = (GenericValue) context.get("userLogin"); >> >> - String partyId = ServiceUtil.getPartyIdCheckSecurity(userLogin, >> security, context, result, "PARTYMGR", "_CREATE"); >> + ServiceUtil.getPartyIdCheckSecurity(userLogin, security, context, >> result, "PARTYMGR", "_CREATE"); >> >> if (result.size() > 0) >> return result; >> @@ -85,4 +92,68 @@ >> result.put(ModelService.RESPONSE_MESSAGE, >> ModelService.RESPOND_SUCCESS); >> return result; >> } >> + >> + /** Creates and updates a PartyRelationship creating related PartyRoles >> if needed. >> + * A side of the relationship is checked to maintain history >> + *...@param ctx The DispatchContext that this service is operating in >> + *...@param context Map containing the input parameters >> + *...@return Map with the result of the service, the output parameters >> + */ >> + public static Map<String, Object> >> createUpdatePartyRelationshipAndRoles(DispatchContext ctx, Map<String, ? >> extends Object> context) { >> + Map<String, Object> result = FastMap.newInstance(); >> + Delegator delegator = ctx.getDelegator(); >> + LocalDispatcher dispatcher = ctx.getDispatcher(); >> + >> + try { >> + List<GenericValue> partyRelationShipList = >> PartyRelationshipHelper.getActivePartyRelationships(delegator, context); > > Ship in relationship shouldn't be camel cased > >> + if (UtilValidate.isEmpty(partyRelationShipList)) { // If >> already exists and active nothing to do: keep the current one >> + String partyId = (String) context.get("partyId") ; >> + String partyIdFrom = (String) context.get("partyIdFrom") ; >> + String partyIdTo = (String) context.get("partyIdTo") ; >> + String roleTypeIdFrom = (String) >> context.get("roleTypeIdFrom") ; >> + String roleTypeIdTo = (String) context.get("roleTypeIdTo") ; >> + String partyRelationshipTypeId = (String) >> context.get("partyRelationshipTypeId") ; >> + >> + // Before creating the partyRelationShip, create the >> partyRoles if they don't exist >> + GenericValue partyToRole = null; >> + partyToRole = delegator.findOne("PartyRole", >> UtilMisc.toMap("partyId", partyIdTo, "roleTypeId", roleTypeIdTo), false); >> + if (partyToRole == null) { >> + partyToRole = delegator.makeValue("PartyRole", >> UtilMisc.toMap("partyId", partyIdTo, "roleTypeId", roleTypeIdTo)); >> + partyToRole.create(); > > You should always use the services provided to create entities and not create > the records yourself > >> + } >> + >> + GenericValue partyFromRole= null; >> + partyFromRole = delegator.findOne("PartyRole", >> UtilMisc.toMap("partyId", partyIdFrom, "roleTypeId", roleTypeIdFrom), false); >> + if (partyFromRole == null) { >> + partyFromRole = delegator.makeValue("PartyRole", >> UtilMisc.toMap("partyId", partyIdFrom, "roleTypeId", roleTypeIdFrom)); >> + partyFromRole.create(); >> + } >> + >> + // Check if there is already a partyRelationship of that >> type with another party from the side indicated >> + String sideChecked = partyIdFrom.equals(partyId)? >> "partyIdFrom" : "partyIdTo"; >> + partyRelationShipList = >> delegator.findByAnd("PartyRelationship", UtilMisc.toMap(sideChecked, partyId, >> + "roleTypeIdFrom", roleTypeIdFrom, >> + "roleTypeIdTo", roleTypeIdTo, >> + "partyRelationshipTypeId", >> partyRelationshipTypeId)); >> + // We consider the last one (in time) as sole active (we >> try to maintain a unique relationship and keep changes history) >> + partyRelationShipList = >> EntityUtil.filterByDate(partyRelationShipList); >> + GenericValue oldPartyRelationShip = >> EntityUtil.getFirst(partyRelationShipList); >> + if (UtilValidate.isNotEmpty(oldPartyRelationShip)) { >> + >> oldPartyRelationShip.setFields(UtilMisc.toMap("thruDate", >> UtilDateTime.nowTimestamp())); // Current becomes inactive >> + oldPartyRelationShip.store(); > > Should be using the crud services but this also seems strange, it looks like > your expiring the existing PartyRelationship by only considering one party > rather than both? > >> + } >> + try { >> + dispatcher.runSync("createPartyRelationship", context); >> // Create new one > > I'm guessing this will fail if a partyId was supplied to the calling service > since createPartyRelationship doesn't take a parameter named partyId. If you > want to do a passthru you should really get the service model and call the > makeValid instance method. > >> + } catch (GenericServiceException e) { >> + Debug.logWarning(e.getMessage(), module); >> + return ServiceUtil.returnError("Could not create party >> relationship (write failure): " + e.getMessage()); >> + } >> + } >> + } catch (GenericEntityException e) { >> + Debug.logWarning(e.getMessage(), module); >> + return ServiceUtil.returnError("Could not create party >> relationship (write failure): " + e.getMessage()); >> + } >> + result.put(ModelService.RESPONSE_MESSAGE, >> ModelService.RESPOND_SUCCESS); >> + return result; >> + } >> } >> >> Added: >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyTypeHelper.java >> URL: >> http://svn.apache.org/viewvc/ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyTypeHelper.java?rev=893961&view=auto >> ============================================================================== >> --- >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyTypeHelper.java >> (added) >> +++ >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyTypeHelper.java >> Sat Dec 26 11:21:21 2009 >> @@ -0,0 +1,56 @@ >> +/******************************************************************************* >> + * Licensed to the Apache Software Foundation (ASF) under one >> + * or more contributor license agreements. See the NOTICE file >> + * distributed with this work for additional information >> + * regarding copyright ownership. The ASF licenses this file >> + * to you under the Apache License, Version 2.0 (the >> + * "License"); you may not use this file except in compliance >> + * with the License. You may obtain a copy of the License at >> + * >> + * http://www.apache.org/licenses/LICENSE-2.0 >> + * >> + * Unless required by applicable law or agreed to in writing, >> + * software distributed under the License is distributed on an >> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY >> + * KIND, either express or implied. See the License for the >> + * specific language governing permissions and limitations >> + * under the License. >> + >> *******************************************************************************/ >> + >> +package org.ofbiz.party.party; >> + >> +import org.ofbiz.base.util.Debug; >> +import org.ofbiz.base.util.UtilMisc; >> +import org.ofbiz.entity.Delegator; >> +import org.ofbiz.entity.GenericEntityException; >> +import org.ofbiz.entity.GenericValue; >> +import org.ofbiz.entity.util.EntityTypeUtil; >> + >> +/** >> + * PartyTypeHelper >> + */ >> +public class PartyTypeHelper { >> + >> + public static final String module = PartyTypeHelper.class.getName(); >> + >> + /** Check if a related party is of the right party type (PERSON or >> PARTY_GROUP) >> + *...@param delegator needed Delegator >> + *...@param partyId a a valid Party Id string >> + *...@param checkedPartyType a string in {PERSON, PARTY_GROUP} >> + *...@return Boolean, false in case of error >> + */ >> + public static Boolean checkPartyType(Delegator delegator, String >> partyId, String checkedPartyType) { >> + GenericValue party = null; >> + GenericValue partyType = null; >> + GenericValue checkedTypeOfParty = null; >> + try { >> + party = delegator.findOne("Party", UtilMisc.toMap("partyId", >> partyId), false); > > The cache could be used here since Party.partyType is unlikely to change > >> + partyType = party.getRelatedOneCache("PartyType"); >> + checkedTypeOfParty = delegator.findOne("PartyType", >> UtilMisc.toMap("partyTypeId", checkedPartyType), true); >> + } catch (GenericEntityException e) { >> + Debug.logWarning(e, module); >> + return false; >> + } >> + return EntityTypeUtil.isType(partyType, checkedTypeOfParty); >> + } >> +} >> >> Propchange: >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyTypeHelper.java >> ------------------------------------------------------------------------------ >> svn:eol-style = native >> >> Propchange: >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyTypeHelper.java >> ------------------------------------------------------------------------------ >> svn:keywords = "Date Rev Author URL Id" >> >> Propchange: >> ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyTypeHelper.java >> ------------------------------------------------------------------------------ >> svn:mime-type = text/plain >> >> >
