Allow specifying any attributes for relations

[andersforsgren]

Problem: I want to create a relation between a Work Item and a Build, e.g. "Integrated in build". That is rather straightforward in the api itself: just add a relation with the url of the build. The problem is that to be accepted, the relation must have a name, e.g. "Integrated in build".

This is an example of what I want to do:

    var attrs = new Dictionary<string, object>
    {
        { "name", "Integrated in build" },
        { "comment", "This is a comment" }
    };
    wrapper.Relations.AddLink("ArtifactLink", buildUrl, attrs);

The current impl of Relations.AddLink and WorkItemRelationWrapper ctor looks like this, allowing only the comment attribute for a relation.

    // In WorkItemRelationWrapperCollection.cs: AddLink only takes single string comment parameter
    public void AddLink(string type, string url, string comment)
    {
        AddRelation(new WorkItemRelationWrapper(
            type,
            url,
            comment
        ));
    }  

   // Which calls into...

   // In WorkItemRelationWrapper.cs: 
   public WorkItemRelationWrapper(string type, string url, string comment)
   {
       _relation = new WorkItemRelation()
       {
           Rel = type,
           Url = url,
           Attributes = new Dictionary<string,object> { { "comment", comment } }
       };
   }

Later, the same filter is applied also at the WorkItemRelationWrapperCollection to make absolutely sure that no attributes other than comment can exist.

  _pivotWorkItem.Changes.Add(new JsonPatchOperation()
  {
        Operation = Operation.Add,
        Path = "/relations/-",
        Value = new RelationPatch
        {
             rel = item.Rel,
             url = item.Url,
             attributes = item.Attributes != null && item.Attributes.TryGetValue("comment", out object value)
                 ? new { comment = value }
                 : null
        }
  });

I propose an overload that accepts any attribute set, like so

   public void AddLink(string type, string url, Dictionary<string, object> attributes) 
   {
       AddRelation(new WorkItemRelationWrapper(
           type,
           url,
           attributes
       ));
   }

I have tested this and it seems to work perfectly. It's not clear however if the current limitation to only a comment attribute has a reason that would make this overload a bad idea to add.

I can make a PR addinig the new overload.

[giuliov]

I need some time to look at the code change (I am catching up backlog these days), in the meantime I want to explain why you see additional checks.

One of the principle behind (Tfs)Aggregator is to help users and prevent them to shoot on their feet. Over the year, we found that people writing rules are not always developers. They can be in a Project Manager/Scrum Master/PMO role, with no familiarity with C# or the idiosyncrasies of the API.

[andersforsgren]

If the full-dictionary-of-attributes is risky for consumers perhaps there is some other way to specify more parameters such as “name”?

At least those parameters that the API requires need to be settable somehow, otherwise many relations can’t be created at all.

An overload with name+comment parameters, for example? (Would be good to have a list of attributes that may be required, and ensure those can be specified. It may be as simple as "name" being the only one).

[BobSilent]

Maybe we can extend our API and distinguish here between WorkItem Relations and "external" Links like build, testresult, any http url etc.

so and idea would be, that we could have the existing

public void AddLink(string type, string url, string comment)

and a dedicated one for non work item links

public void AddExternalLink(string type, string url, string name, string comment = null)

at the end we should improve documentation here on how to handle and what needs to be specified.
I agree that the regular user will use workitem links, this would work as before. But in some situations you need a little bit more expert usage and than you also (hopefully) read the documentation on how to do it.

[andersforsgren]

Sounds good.

To make this even simpler/safer (in terms of choosing the right method) one could provide public methods for the different types of links: E.g ArtifactLinks which have a fixed specific type (rel) and it’s when that type is used that the name attribute is always required.

public void AddArtifactLink(string url, string name,  string comment = null) 
{ 
   if (string.IsNullOrEmpty(nameof(name))
     throw new ArgumentException($”{nameof(name)} is required”)

    // This can be internal 
    AddLink(“ArtifactLink”, url, name, comment);
}

AddArtifactLink(“vstfs://Build/Build/123”, “Integrated in Build”);

On a related note, I was bitten by the fact that the actual patch call to the server swallows any errors returned. E.g the Save changes method returns “0 created, 1 updated” but nothing actually changed. Stepping through it I noticed the api response was “400: Server explanation of why it won’t perform that update”. Could e.g be permissions, rules etc. Adding a line to log any non-200 responses in the save changes made everything much less frustrating! This should perhaps be a separate issue.