Add multi object test #87
Conversation
| yield return new TestCaseData("SingleChoice", SingleChoiceFieldChoices.SingleChoice2); | ||
| yield return new TestCaseData("GravityLevel2Obj", new GravityLevel2() { Name = "Test_" + Guid.NewGuid() }); | ||
| yield return new TestCaseData("GravityLevel2MultipleObjs", new List<GravityLevel2>(){ | ||
| new GravityLevel2() |
There was a problem hiding this comment.
Consider using a LINQ statement to get this done. Something like
Enumerable.Range(1,3).Select(_ => new GravityLevel2 { Name = "Test_" + Guid.NewGuid() }).ToList()There was a problem hiding this comment.
Done, will look at moving these test cases in RSAPI_IntegrationTests.cs and you have with the other unit tests in another PR.
| @@ -34,15 +34,9 @@ public class GravityLevelOne : BaseDto | |||
| public RelativityFile FileField { get; set; } | |||
|
|
|||
| [RelativityObjectField("D0770889-8A4D-436A-9647-33419B96E37E", RdoFieldType.MultipleObject, typeof(GravityLevel2))] | |||
| public IList<int> GravityLevel2MultipleArtifactIds { get; set; } | |||
|
|
|||
| [RelativityMultipleObject("D0770889-8A4D-436A-9647-33419B96E37E")] | |||
There was a problem hiding this comment.
These are the wrong removed attributes, no? The types on the remaining object versions should stay as is (RelativityMultipleObject, RelativitySingleObject).
There was a problem hiding this comment.
At first that is the route I took, but when I looked at it, there is no real need to the single objects and multiple object as anything other than a different field type. I have a feeling child object will be similar. This is much easier for the consumer. One way to define all the fields.
There was a problem hiding this comment.
Well, child object can't be similar, because it doesn't even have a field definition.
As for single/multiple, the idea is that you don't need to declare the type because it can be inferred. Perhaps we should remove the type argument on RelativityObjectField altogether, now that we don't have the ID-list versions.
There was a problem hiding this comment.
I realized I need to update the Readme file as well. Will do that in another PR
There was a problem hiding this comment.
Why another one? If you're changing the contract, you should do it in this one.
Similarly, I think this PR should remove the ObjectFieldDTOType attribute, as mentioned in a previous comment.
There was a problem hiding this comment.
Why another one? I was trying to rush to get this done, but you are right. I updated the readme.
As far as the remove ObjectFieldDTOType...I think that should be done. However I would like to save that for another PR. I will create an issue for it.
| gravityFieldValue = (gravityFieldValue as GravityLevel2).Name; | ||
| break; | ||
| case RdoFieldType.MultipleObject: | ||
| var resultsDictionary = new Dictionary<int, string>(); |
There was a problem hiding this comment.
you and Linq? I gotta admit, it's starting to grow on me a bit. Done.
|
The now-unused |
There was a problem hiding this comment.
I really like this; much simpler! Just a few changes suggested (I had extra time allotted to day on Gravity 😉)
Also see these comments:
#87 (comment)
#87 (comment)
Gravity/Gravity/Base/BaseDto.cs
Outdated
| @@ -187,7 +137,15 @@ private static object ConvertPropertyValue(PropertyInfo property, RdoFieldType f | |||
| case RdoFieldType.MultipleObject: | |||
| { | |||
| return new FieldValueList<Artifact>( | |||
| ((IList<int>)propertyValue).Select(x => new Artifact(x))); | |||
| ( | |||
There was a problem hiding this comment.
Cast objects to BaseDto instead of using reflection (yes, this issue preceded you). Also align parentheses better
There was a problem hiding this comment.
very good call. I had the same thought about reflection being expensive and taking a pass through the code looking for ways to optimize or potentially using caching or something. No need to continue to contribute to the problem. Done
Gravity/Gravity/Base/BaseDto.cs
Outdated
| @@ -202,9 +160,11 @@ private static object ConvertPropertyValue(PropertyInfo property, RdoFieldType f | |||
|
|
|||
| case RdoFieldType.SingleObject: | |||
| { | |||
| if ((int)propertyValue > 0) | |||
| int artifactId = (int)propertyValue.GetType().GetProperty("ArtifactId")?.GetValue(propertyValue, null); | |||
There was a problem hiding this comment.
Cast object to BaseDto instead of using reflection (yes, this issue preceded you).
| @@ -118,13 +120,15 @@ internal void PopulateChildrenRecursively<T>(BaseDto baseDto, RDO objectRdo, Obj | |||
|
|
|||
| private object GetChildObjectRecursively(BaseDto baseDto, RDO objectRdo, ObjectFieldsDepthLevel depthLevel, PropertyInfo property) | |||
| { | |||
| var fieldType = property.GetCustomAttribute<RelativityObjectFieldAttribute>()?.FieldType; | |||
There was a problem hiding this comment.
Instead of this, try and find the RelativityObjectFieldAttribute and wrap the next two blocks in an "if not null" statement. We shouldn't be looking up the attribute twice in a row (reflection is expensive), and it better communicates intent (the FieldGuid can never be null when the field type is set).
| private bool InsertUpdateSingleObjectFields(BaseDto objectToInsert) | ||
| { | ||
| foreach (var propertyInfo in objectToInsert.GetType().GetProperties().Where(c => | ||
| c.GetCustomAttribute<RelativityObjectFieldAttribute>()?.FieldType == RdoFieldType.SingleObject)) |
There was a problem hiding this comment.
Don't query for the attribute twice. Try and find it, and move on if it is null
There was a problem hiding this comment.
@Arithmomaniac Not sure what you mean here? I need to get all the SingleObject field types, then do stuff with those fields. Suggestions requested for clarity.
There was a problem hiding this comment.
foreach (var propertyInfo in objectToInsert.GetType().GetProperties())
{
var attribute = propertyInfo.GetCustomAttribute<RelativityObjectFieldAttribute>();
if (attribute?.FieldType == RdoFieldType.SingleObject)
{
var fieldValue = (BaseDto)objectToInsert.GetPropertyValue(propertyInfo.Name);
if (fieldValue != null)
{
var fieldGuid = attribute.FieldGuid;
//...
}
}
}| c.GetCustomAttribute<RelativityObjectFieldAttribute>()?.FieldType == RdoFieldType.SingleObject)) | ||
| { | ||
| var fieldGuid = propertyInfo.GetCustomAttribute<RelativityObjectFieldAttribute>()?.FieldGuid; | ||
| var fieldValue = (objectToInsert.GetPropertyValue(propertyInfo.Name) as BaseDto); |
There was a problem hiding this comment.
Cast instead of using as
|
|
||
| if (fieldGuid != null && fieldValue != null) | ||
| { | ||
| //TODO: better test to see if contains value...if all fields are null, not need |
There was a problem hiding this comment.
I'd insert it anyways; expected behavior is to insert an object if you ask for it (no different than, say, what would happen with RSAPI today).
There was a problem hiding this comment.
Good logical thinking as always. Cleaned it up.
| } | ||
| else | ||
| { | ||
| //TODO: Consider update if fields have changed |
There was a problem hiding this comment.
If we're not updating for now, I would just name the base method InsertSingleObjectFields.
| } | ||
|
|
||
| public int InsertRelativityObject<T>(BaseDto theObjectToInsert) | ||
| private bool InsertUpdateMultipleObjectFields(BaseDto objectToInsert) |
There was a problem hiding this comment.
Comments above apply here too
| { | ||
| foreach (var childObject in fieldValue) | ||
| { | ||
| if (fieldGuid != null && fieldValue != null) |
There was a problem hiding this comment.
This check is redundant
…ect changes. Also cleand up code from code comments.
|
Good suggestions as usual. Made most of them. The only open item from comments is the multiple query stuff around Insert.cs around line 130. |
…s per code review comments
|
@Arithmomaniac, didn't see your latest comment last week for some reason. Thanks for posting. The latest commit cleans up the last of this round of comments. Should be good to go here. |
I know there are some tab and spacing redos, but I am now using the extension Avi recommended. The Write test does not pass yet, but I will work on both single and multi object write tests next.