Fixes #1621 NewRepositoryWebHook no longer creates new instance... (#1623)

Fixes #1621 NewRepositoryWebHook should not discard existing fields

Octokit.Tests/Models/NewRepositoryWebHookTests.cs

@@ -7,9 +7,6 @@ namespace Octokit.Tests.Models
     {
         public class TheCtor
         {
-            string ExpectedRepositoryWebHookConfigExceptionMessage =
-                "Duplicate webhook config values found - these values: Url should not be passed in as part of the config values. Use the properties on the NewRepositoryWebHook class instead.";
-
             [Fact]
             public void UsesDefaultValuesForDefaultConfig()
             {
@@ -78,7 +75,67 @@ namespace Octokit.Tests.Models
             }
 
             [Fact]
-            public void ShouldThrowRepositoryWebHookConfigExceptionWhenDuplicateKeysExists()
+            public void CanSetHookAsActive()
+            {
+                var config = new Dictionary<string, string>
+                {
+                    {"hostname", "http://hostname.url"},
+                    {"username", "username"},
+                    {"password", "password"}
+                };
+
+                var create = new NewRepositoryWebHook("web", config, "http://test.com/example")
+                {
+                    Active = true
+                };
+
+                var request = create.ToRequest();
+
+                Assert.True(request.Active);
+            }
+
+            [Fact]
+            public void CanSetHookEvents()
+            {
+                var create = new NewRepositoryWebHook("web", new Dictionary<string, string>(), "http://test.com/example")
+                {
+                    Events = new List<string> { "*" }
+                };
+
+                var request = create.ToRequest();
+
+                Assert.Contains("*", request.Events);
+            }
+
+            [Fact]
+            public void EnsureCanCallToRequestMultipleTimes()
+            {
+                var create = new NewRepositoryWebHook("web", new Dictionary<string, string>(), "http://test.com/example")
+                {
+                    Events = new List<string> { "*" }
+                };
+
+                var request = create.ToRequest();
+                var requestRepeated = create.ToRequest();
+
+                Assert.Contains("*", request.Events);
+                Assert.Contains("*", requestRepeated.Events);
+            }
+
+            [Fact]
+            public void ShouldNotContainDuplicateConfigEntriesOnSubsequentRequests()
+            {
+                var create = new NewRepositoryWebHook("web", new Dictionary<string, string>(), "http://test.com/example");
+
+                var request = create.ToRequest();
+                var requestRepeated = create.ToRequest();
+
+                Assert.Equal(request.Config.Count, 4);
+                Assert.Equal(requestRepeated.Config.Count, 4);
+            }
+
+            [Fact]
+            public void ShouldNotContainDuplicateConfigEntriesOnSubsequentRequestsWithCustomisedConfig()
             {
                 var config = new Dictionary<string, string>
                 {
@@ -88,20 +145,32 @@ namespace Octokit.Tests.Models
                     {"password", "password"}
                 };
 
-                var create = new NewRepositoryWebHook("windowsazure", config, "http://test.com/example")
+                var create = new NewRepositoryWebHook("web", config, "http://test.com/example");
+
+                var request = create.ToRequest();
+                var requestRepeated = create.ToRequest();
+
+                //This is not 8, because `url` used in config, is already part of the base config
+                Assert.Equal(request.Config.Count, 7);
+                Assert.Equal(requestRepeated.Config.Count, 7);
+            }
+
+            [Fact]
+            public void PropertiesShouldTakePrecedenceOverConfigPassedIn()
+            {
+                var config = new Dictionary<string, string>
                 {
-                    ContentType = WebHookContentType.Json,
-                    Secret = string.Empty,
-                    InsecureSsl = true
+                    {"url", "http://originalurl.com/test"},
                 };
 
-                Assert.Equal(create.Url, "http://test.com/example");
-                Assert.Equal(create.ContentType, WebHookContentType.Json);
-                Assert.Empty(create.Secret);
-                Assert.True(create.InsecureSsl);
+                var create = new NewRepositoryWebHook("web", config, "http://test.com/example");
 
-                var ex = Assert.Throws<RepositoryWebHookConfigException>(() => create.ToRequest());
-                Assert.Equal(ExpectedRepositoryWebHookConfigExceptionMessage, ex.Message);
+                var request = create.ToRequest();
+
+                Assert.Equal(request.Config["url"], "http://test.com/example");
+
+                var subsequentRequest = create.ToRequest();
+                Assert.Equal(subsequentRequest.Config["url"], "http://test.com/example");
             }
         }
     }

Octokit/Exceptions/RepositoryWebHookConfigException.cs

@@ -1,62 +0,0 @@
-using System;
-using System.Collections.Generic;
-using System.Diagnostics.CodeAnalysis;
-using System.Globalization;
-using System.Linq;
-#if !NO_SERIALIZABLE
-using System.Runtime.Serialization;
-#endif
-using System.Security;
-
-namespace Octokit
-{
-#if !NO_SERIALIZABLE
-    [Serializable]
-#endif
-    [SuppressMessage("Microsoft.Design", "CA1032:ImplementStandardExceptionConstructors",
-        Justification = "These exceptions are specific to the GitHub API and not general purpose exceptions")]
-    public class RepositoryWebHookConfigException : Exception
-    {
-        readonly string message;
-
-        public RepositoryWebHookConfigException(IEnumerable<string> invalidConfig)
-        {
-            var parameterList = string.Join(", ", invalidConfig.Select(ic => ic.FromRubyCase()));
-            message = string.Format(CultureInfo.InvariantCulture,
-                "Duplicate webhook config values found - these values: {0} should not be passed in as part of the config values. Use the properties on the NewRepositoryWebHook class instead.",
-                parameterList);
-        }
-
-        public override string Message
-        {
-            get { return message; }
-        }
-
-#if !NO_SERIALIZABLE
-        /// <summary>
-        /// Constructs an instance of RepositoryWebHookConfigException
-        /// </summary>
-        /// <param name="info">
-        /// The <see cref="SerializationInfo"/> that holds the
-        /// serialized object data about the exception being thrown.
-        /// </param>
-        /// <param name="context">
-        /// The <see cref="StreamingContext"/> that contains
-        /// contextual information about the source or destination.
-        /// </param>
-        protected RepositoryWebHookConfigException(SerializationInfo info, StreamingContext context)
-            : base(info, context)
-        {
-            if (info == null) return;
-            message = info.GetString("Message");
-        }
-
-        [SecurityCritical]
-        public override void GetObjectData(SerializationInfo info, StreamingContext context)
-        {
-            base.GetObjectData(info, context);
-            info.AddValue("Message", Message);
-        }
-#endif
-    }
-}

Octokit/Models/Request/NewRepositoryHook.cs

@@ -81,7 +81,7 @@ namespace Octokit
         /// <value>
         /// The configuration.
         /// </value>
-        public IReadOnlyDictionary<string, string> Config { get; private set; }
+        public IReadOnlyDictionary<string, string> Config { get; protected set; }
 
         /// <summary>
         /// Determines what events the hook is triggered for. Default: ["push"]

Octokit/Models/Request/NewRepositoryWebHook.cs

@@ -32,8 +32,8 @@ namespace Octokit
     /// <item>
     ///   <term>insecure_ssl:</term>
     ///   <description>
-    ///     An optional string that determines whether the SSL certificate of the host for url will be verified when 
-    ///     delivering payloads. Supported values include "0" (verification is performed) and "1" (verification is not 
+    ///     An optional string that determines whether the SSL certificate of the host for url will be verified when
+    ///     delivering payloads. Supported values include "0" (verification is performed) and "1" (verification is not
     ///     performed). The default is "0".
     ///   </description>
     /// </item>
@@ -41,16 +41,16 @@ namespace Octokit
     /// <para>
     /// API: https://developer.github.com/v3/repos/hooks/#create-a-hook
     /// </para>
-    /// </remarks>   
+    /// </remarks>
     [DebuggerDisplay("{DebuggerDisplay,nq}")]
     public class NewRepositoryWebHook : NewRepositoryHook
     {
         /// <summary>
-        /// Initializes a new instance of the <see cref="NewRepositoryWebHook"/> class.   
-        /// Using default values for ContentType, Secret and InsecureSsl.     
+        /// Initializes a new instance of the <see cref="NewRepositoryWebHook"/> class.
+        /// Using default values for ContentType, Secret and InsecureSsl.
         /// </summary>
         /// <param name="name">
-        /// Use "web" for a webhook or use the name of a valid service. (See 
+        /// Use "web" for a webhook or use the name of a valid service. (See
         /// <see href="https://api.github.com/hooks">https://api.github.com/hooks</see> for the list of valid service
         /// names.)
         /// </param>
@@ -99,29 +99,22 @@ namespace Octokit
         public string Secret { get; set; }
 
         /// <summary>
-        /// Gets whether the SSL certificate of the host will be verified when 
+        /// Gets whether the SSL certificate of the host will be verified when
         /// delivering payloads. The default is `false`.
         /// </summary>
         /// <value>
-        ///  <c>true</c> if SSL certificate verification is not performed; 
+        ///  <c>true</c> if SSL certificate verification is not performed;
         /// otherwise, <c>false</c>.
         /// </value>
         public bool InsecureSsl { get; set; }
 
         public override NewRepositoryHook ToRequest()
         {
-            var webHookConfig = GetWebHookConfig();
-            if (Config.Any(c => webHookConfig.ContainsKey(c.Key)))
-            {
-                var invalidConfigs = Config.Where(c => webHookConfig.ContainsKey(c.Key)).Select(c => c.Key);
-                throw new RepositoryWebHookConfigException(invalidConfigs);
-            }
-
-            var config = webHookConfig
+            Config = GetWebHookConfig()
                 .Union(Config, new WebHookConfigComparer())
                 .ToDictionary(k => k.Key, v => v.Value);
 
-            return new NewRepositoryHook(Name, config);
+            return this;
         }
 
         Dictionary<string, string> GetWebHookConfig()