Add ASP.NET Core WebHooks support (issue #5) #153

dougbu · 2017-08-27T07:31:52Z

This approach is more ASP.NET Core-native than #152.

Users write controller actions annotated with (say) [GitHubWebHookAction] instead of WebHookHandlers. Dispatching between multiple handlers must be done manually though we could add abstract controller base classes which dispatch to handlers in the old way.

Those developing new receivers can start from WebHookReceiver though the subclasses should also implement IResourceFilter or IAsyncResourceFilter. This improves modularity and allows base WebHook...Filters for most needs.

dnfclas · 2017-08-27T07:31:57Z

@dougbu,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

davidfowl · 2017-09-13T08:47:56Z

src/Microsoft.AspNetCore.WebHooks.Common/Extensions/DictionaryExtensions.cs

+
+using System.ComponentModel;
+
+namespace System.Collections.Generic


Why are these exposed as public API on types we don't own? This should be shared source

Absolutely. This was straight out of Microsoft.AspNet.WebHooks.Common and I didn't plan to ship with the extension methods in place.

Could skip creating a shared-source package and make these non-extension static methods in a Microsoft.AspNetCore.WebHooks namespace. The methods are used in various receivers but I'm not sure user code (controller actions in the new world) need them.

davidfowl · 2017-09-13T08:48:14Z

src/Microsoft.AspNetCore.WebHooks.Common/Extensions/StringExtensions.cs

+using System.ComponentModel;
+using System.Linq;
+
+namespace System


Same comment as above

Havret · 2017-10-05T20:08:43Z

What is the status of this PR? Can I safely try to use it?

dougbu · 2017-10-05T20:34:21Z

@Havret our team feels the general approach is sound but we have not yet reviewed the details of this PR. Feel free to test it out but I would not recommend call it a perfect indicator of the APIs we'll ship. Put another way, treat dougbu/add.core.II as a proposed feature branch with more changes expected.

Note the Core Trello and WordPress receivers are currently almost-empty projects. Core Custom, Generic, Instagram, MyGet, VSTS, and Zendesk receivers do not yet exist; neither does anything on the sender side (no Microsoft.AspNetCore.WebHooks.Custom…).

Please let us know if you experience any problems!

For those of you used to the WebHooks 1.0.0 approach:

Receiver names shouldn’t have changed
Configuration settings names and formats shouldn’t have changed though web.config isn’t relevant anymore
- best set as Azure application settings, environment variables, or in appsettings.json
Handlers are now ASP.NET Core MVC action methods with [GitHubWebHook] attributes (or whichever receiver you choose)
- check the attribute documentation comments for supported action parameters; names are important here because binding is convention-based
A number of receivers do not support binding event names but KuduConstants (or whichever) and IFormCollection, JObject, and XElement action parameters make it pretty each to grab the information from request bodies
Receivers are now a bundle of metadata-driven MVC application models, constraints, and filters that are (mostly) enabled globally but no-op for receivers that don’t use them
- configure a receiver in your Startup.ConfigureServices(…) method with services.AddMvcCore().AddAzureAlertWebHooks(); (or whichever)

dougbu · 2017-10-05T21:51:07Z

@Havret I'm not sure what you mean. The branch contains a fair number of different Core receivers as well as sample ASP.NET Core MVC applications using them. Could you clarify your question?

Havret · 2017-10-05T22:12:06Z

@dougbu What's missing is the example how to configure custom receiver which can consume events send by old asp.net publisher. It would be great if you can add something like that. Because lack of documentation I'm having problems with picking up the pieces. Cheers.

dougbu · 2017-10-08T03:36:33Z

Rebased on dougbu/stripe.event.data.154 and removed changes covered in #161. Now includes Stripe support in Core receiver and command-line builds are clean.

- other than `Resources` classname, basically unchanged from approach 1

- fill in `WebHookVerifyMethodFilter` - make `[WebHookAction]` a `[consumes]` subclass

- no more `internal const` fields in Core projects - `WebHookRouteNames` -> `WebHookConstants` and move up to `Microsoft.AspNetCore.WebHooks` namespace

- apply Resources.resx changes in all Core projects - extend `$(PackageOutputPath)` change to include `$(PlatformName)` (if needed) - remove `_SetPackageOutputPath` target; no need for additional delay - add `$(IsTestProject)` setting to test/Directory.Build.props nit: consistent whitespace

- remove unused resources - make names and values more consistent

nit: minor doc comment consistency improvement - also add `CreateErrorResult(ModelStateDictionary)` to WebHookResultUtilities nit: correct some doc comments - address a TODO item - improve logging - add resources for thrown `Exception`s nits: - improve a couple of other resources - wrap entire `Exception` caught in `WebHookReceiverConfig` rather than include inner `Message` in outer one - not currently used - if needed, `DictionaryExtensions` could come back as a shared source project or regular static methods - add `TrimmingTokenizer` to replace `StringExtensions` features - add TODO items - remove ??? comments for answered or less-important questions - use `WebHookErrorKeys.MessageKey` as `BinderModelName` when binding from body - add missing `null` checks nit: add missing `<returns>` - add `SalesforceAcknowledgmentFilter` - wire up `SalesforceMvcCoreBuilderExtensions` - short-circuit `SalesforceVerifyOrganizationIdFilter` when it doesn't apply; call `next()` on happy paths - simplify `SalesforceMetadata` - use `<see cref="xyz"/>` for type short names; these aren't `langword`s - use `<see langword="xyz"/>` for `false`, `null` and `true` - use `<see href="xyz"/>` for links - simplify `<list>`s - currently builds cleanly but does not produce Microsoft.AspNetCore.WebHooks.* packages - ignore intermediate `.nuspec` files created for Core projects nits: - filter consistency e.g. early exit more, nest code less - add `WebHookReceiverConfig.IsTrue(...)` extension method - `nameof(AType)` is simpler than `typeof({AType}).Name` - fix a few older comments - simplify Salesforce receiver's `[Produces]` - use `https` - Slack and Stripe receivers too

dougbu · 2017-10-16T23:39:23Z

🆙📅 to reduce number of commits

rynowak · 2017-10-19T19:54:07Z

src/Microsoft.AspNetCore.WebHooks.Common/Extensions/WebHookHttpRequestExtensions.cs

+            return false;
+        }
+
+        // ??? Should ASP.NET Core expose a similar HttpRequest, HttpContext or ConnectionInfo (extension) method?


// ??? Should ASP.NET Core expose a similar HttpRequest, HttpContext or ConnectionInfo (extension) method? [](start = 8, length = 106)

???

Wondering if this extension should be in the ASP.NET Core box.

Open a bug 👍 - I don't think this is the right place for this. If we need this code we should inline it or make it internal

In reply to: 145833956 [](ancestors = 145833956)

Removed the IsLocal() extension method. IsLocal() is no longer a recommended approach to relaxing security requirements.

rynowak · 2017-10-19T19:55:03Z

src/Microsoft.AspNetCore.WebHooks.Receivers/GeneralWebHookAttribute.cs

+    /// <see cref="WebHookAttribute.Id"/>. Also adds a <see cref="Filters.WebHookReceiverExistsFilter"/> for
+    /// the action.
+    /// </summary>
+    public class GeneralWebHookAttribute : WebHookAttribute, IWebHookRequestMetadata, IWebHookEventSelectorMetadata


GeneralWebHookAttribute [](start = 17, length = 23)

This is a cool idea, might want to call it GenericWebHookAttribute

WebHooks already contains a Microsoft.AspNet.WebHooks.Receivers.Generic package and GenericJsonWebHookReceiver. So, I avoided GenericWebHookAttribute here because it would be too close to an eventual GenericJsonWebHookAttribute.

How about AllConfiguredWebHookAttribute? Associated actions don't handle everything, just requests for configured receivers.

rynowak · 2017-10-19T19:57:30Z

src/Microsoft.AspNetCore.WebHooks.Receivers/Microsoft.AspNetCore.WebHooks.Receivers.csproj

+  <ItemGroup>
+    <PackageReference Include="Microsoft.AspNetCore.Mvc.Core" Version="2.0.0" />
+    <PackageReference Include="Microsoft.AspNetCore.Mvc.Formatters.Json" Version="2.0.0" />
+    <PackageReference Include="Microsoft.AspNetCore.Mvc.Formatters.Xml" Version="2.0.0" />


[](start = 4, length = 86)

What's the piece that requires the JSON and XML formatters specifically?

This is really about project file duplication. Most of the receiver-specific projects call AddJsonFormatters(), AddXmlSerializerFormatters() or both. But, if you prefer, I could move the dependency into those projects. LMK

Moved the package references to the receiver-specific projects.

rynowak · 2017-10-19T19:58:16Z

src/Microsoft.AspNetCore.WebHooks.Receivers/WebHookAttribute.cs

+    /// <see cref="WebHookReceiverExistsFilter"/> for the action.
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
+    public abstract class WebHookAttribute : Attribute, IAllowAnonymous, IFilterFactory


IAllowAnonymous [](start = 56, length = 15)

Are webhooks ALWAYS anonymous?

rynowak · 2017-10-19T19:59:12Z

src/Microsoft.AspNetCore.WebHooks.Receivers/WebHookAttribute.cs

+        /// <summary>
+        /// Gets the name of an available <see cref="IWebHookReceiver"/>.
+        /// </summary>
+        public string ReceiverName { get; }


public string ReceiverName { get; } [](start = 8, length = 35)

Is this always required? A subclass that uses the default constructor can't set it

Yes, a receiver-specific attribute needs a receiver-specific non-null name. WebHookMetadataProvider checks this property to make sure it's not null except in an attribute that also implements IWebHookRequestMetadata. But, there isn't a check to make sure the attribute is [GeneralWebHook].

Easiest thing to do may be to make the default constructor above internal. Thoughts?

Made the default constructor internal.

rynowak · 2017-10-19T21:33:44Z

src/Microsoft.AspNetCore.WebHooks.Receivers/WebHookOptions.cs

+        /// Default collection includes <see cref="IFormCollection"/>, <see cref="JArray"/>, <see cref="JObject"/>, and
+        /// <see cref="XElement"/>.
+        /// </value>
+        public IList<Type> HttpContextItemsTypes { get; } = new List<Type>


HttpContextItemsTypes [](start = 27, length = 21)

I don't super know what this is about... I feel like there's a feature here that needs a better explanation

This is about resource filters that call input formatters. When they're done they place the deserialized object into type-named locations in HttpContext.Items for use in the WebHookHttpContextModelBinder. The mechanism overall exists to reduce the number of times the request body is parsed. This exists to avoid leaking random HttpContext.Items entries into the actions and to ensure WebHookModelBindingProvider configures the WebHookHttpContextModelBinder correctly.

What of that information is needed in the doc comments? Or, are you suggesting we do away with either this knob or the overall "read once" mechanism?

This seems like an implementation detail that is masquerading as a feature, we should talk more about this

In reply to: 145838603 [](ancestors = 145838603)

rynowak · 2017-10-19T21:34:37Z

src/Microsoft.AspNetCore.WebHooks.Receivers/WebHookReceiverConfig.cs

+    /// example, '<c>secret0, id1=secret1, id2=secret2</c>'. The corresponding WebHook URI is of the form
+    /// '<c>https://&lt;host&gt;/api/webhooks/incoming/custom/{id}</c>'.
+    /// </summary>
+    public class WebHookReceiverConfig : IWebHookReceiverConfig


WebHookReceiverConfig [](start = 17, length = 21)

What isn't this just IConfiguration. It sounds like there's a feature here that needs to be thought out

The receivers do contain direct references to the IConfiguration. But, this is about specially-formatted configuration entries for secret keys. This uses the same key formats as the Microsoft.AspNet.WebHooks version.

As discussed offline, I'll switch to IConfiguration extension methods supporting a consistent hierarchy of configuration sections e.g.

"WebHooks": { "GitHub": { "SecretKey": { "default": "default secret key", "id1": "secret key for id1" } } }

I don't think I'll make "default" a reserved word because nothing breaks when a URL contains that id. Instead https://{host}/api/webhooks/incoming/github and https://{host}/api/webhooks/incoming/github/default will use the same secret key.

The other option looks like

"WebHooks": { "GitHub": { "SecretKey": { "": "default secret key", "id1": "secret key for id1" } } }

That is valid JSON but looks very strange. And I'm not sure our configuration system supports empty property names.

rynowak · 2017-10-19T21:35:54Z

src/Microsoft.AspNetCore.WebHooks.Receivers/ApplicationModels/WebHookMetadataProvider.cs

+        {
+            _bindingMetadata = metadata.OfType<IWebHookBindingMetadata>().ToArray();
+            _eventMetadata = metadata.OfType<IWebHookEventMetadata>().ToArray();
+            _requestMetadata = metadata.OfType<IWebHookRequestMetadataService>().ToArray();


This seems like it's doing an end-run around DI? Why aren't these things just registered in DI with the approriate types?

Done this way for flexibility. All of the metadata interfaces inherit from IWebHookMetadata and that's how the implementations should be registered. This mechanism allows specific receivers to register a single metadata object once or multiple per-interface instances however much they like.

If I instead had the receivers register using the specific interfaces, could still have one object per receiver that does the implementation. But, the registration would get messy and multiple classes would be aware of the chosen implementations -- the implementation and the extension methods doing registration.

rynowak · 2017-10-23T23:42:53Z

src/Microsoft.AspNetCore.WebHooks.Receivers/WebHookConstants.cs

+        /// <see cref="Filters.WebHookVerifyCodeFilter"/> calls the method if
+        /// <see cref="Metadata.IWebHookSecurityMetadata.VerifyCodeParameter"/> is <see langword="true"/>.
+        /// </remarks>
+        public static string DisableHttpsCheckConfigurationKey => "MS_WebHookDisableHttpsCheck";


public static string DisableHttpsCheckConfigurationKey => "MS_WebHookDisableHttpsCheck"; [](start = 8, length = 88)

What are these keys into, this isn't the way our IConfiguration system typically works

rynowak · 2017-10-23T23:48:01Z

src/Microsoft.AspNetCore.WebHooks.Receivers/WebHookReceiverConfig.cs

+                    key.StartsWith(WebHookConstants.ReceiverConfigurationKeyPrefix, StringComparison.OrdinalIgnoreCase))
+                {
+                    // Extract configuration (again, likely receiver) name
+                    var configurationName = key.Substring(WebHookConstants.ReceiverConfigurationKeyPrefix.Length);


This is interesting, I think this needs some design because stuff like this is pretty high impact and we want it to be consistent across the stack

JDL440 · 2017-10-26T02:47:58Z

Is .NET Core 1.0 a possibility?

dougbu · 2017-10-26T02:55:59Z

@JDL440 this PR targets .NET Core 2.0. What drives your 1.0 request?

JDL440 · 2017-10-26T03:15:48Z

My service targets 1.0 currently and I'm investigating add webhook subscriptions.

dougbu · 2017-10-26T03:40:06Z

@JDL440 please file a separate issue in this repo so that we can track the idea of re-targeting the new ASP.NET Core support.

Another issue, perhaps filed at https://github.com/aspnet/Home, may help us understand anything blocking migrating your service to .NET Core 2.0. For example, is the documentation at https://docs.microsoft.com/en-us/aspnet/core/migration/1x-to-2x/ insufficient in some way?

dougbu

Updates coming…

dougbu · 2017-10-28T17:54:33Z

src/Microsoft.AspNetCore.WebHooks.Common/Extensions/WebHookHttpRequestExtensions.cs

+            return false;
+        }
+
+        // ??? Should ASP.NET Core expose a similar HttpRequest, HttpContext or ConnectionInfo (extension) method?


Removed the IsLocal() extension method. IsLocal() is no longer a recommended approach to relaxing security requirements.

dougbu · 2017-10-28T17:55:53Z

src/Microsoft.AspNetCore.WebHooks.Receivers/Microsoft.AspNetCore.WebHooks.Receivers.csproj

+  <ItemGroup>
+    <PackageReference Include="Microsoft.AspNetCore.Mvc.Core" Version="2.0.0" />
+    <PackageReference Include="Microsoft.AspNetCore.Mvc.Formatters.Json" Version="2.0.0" />
+    <PackageReference Include="Microsoft.AspNetCore.Mvc.Formatters.Xml" Version="2.0.0" />


Moved the package references to the receiver-specific projects.

dougbu · 2017-10-28T17:56:58Z

src/Microsoft.AspNetCore.WebHooks.Receivers/WebHookAttribute.cs

+        /// <summary>
+        /// Gets the name of an available <see cref="IWebHookReceiver"/>.
+        /// </summary>
+        public string ReceiverName { get; }


Made the default constructor internal.

dougbu · 2017-10-28T18:40:48Z

src/Microsoft.AspNetCore.WebHooks.Receivers/WebHookReceiverConfig.cs

+    /// example, '<c>secret0, id1=secret1, id2=secret2</c>'. The corresponding WebHook URI is of the form
+    /// '<c>https://&lt;host&gt;/api/webhooks/incoming/custom/{id}</c>'.
+    /// </summary>
+    public class WebHookReceiverConfig : IWebHookReceiverConfig


As discussed offline, I'll switch to IConfiguration extension methods supporting a consistent hierarchy of configuration sections e.g.

"WebHooks": { "GitHub": { "SecretKey": { "default": "default secret key", "id1": "secret key for id1" } } }

I don't think I'll make "default" a reserved word because nothing breaks when a URL contains that id. Instead https://{host}/api/webhooks/incoming/github and https://{host}/api/webhooks/incoming/github/default will use the same secret key.

The other option looks like

"WebHooks": { "GitHub": { "SecretKey": { "": "default secret key", "id1": "secret key for id1" } } }

That is valid JSON but looks very strange. And I'm not sure our configuration system supports empty property names.

dougbu · 2017-10-30T20:02:04Z

5f62336 got this in because it's too big to review. Will address any issues in follow-up PRs.

- grab some fixes from aspnet/AspNetWebStack repo e.g. parse xUnit test results correctly - revamp `IConfiguration` use - remove `IWebHookReceiverConfig`, its implementation and extension methods - add `IConfiguration` extension methods - make methods synchronous if they were `async` only for configuration lookups - remove configuration value caching in `PusherVerifySignatureFilter` - remove `HttpRequest.IsLocal()` extension method - instead check for development environment - move JSON and XML dependencies into specific receiver projects - make default `WebHookAttribute` constructor `internal` and move its doc comments to `GeneralWebHookAttribute` - add `RouteData.TryGetEventName[s](...)` end `SetWebHookEventNames(...)` extension methods - make most extension names unique to WebHooks - removing `WebHookConfigurationExtensions` entirely is part of #174 nits and smaller TODOs: - remove comments about the default route name; it's `null` everywhere - correct `IResourceFilter` / `IAsyncResourceFilter` comment inconsistencies - remove unused resources - add a few missing `[EditorBrowsable(...)]` attributes on extension classes - remove `WebHookConstants.ReceiverRouteName`

- grab some fixes from aspnet/AspNetWebStack repo e.g. parse xUnit test results correctly - revamp `IConfiguration` use - remove `IWebHookReceiverConfig`, its implementation and extension methods - add `IConfiguration` extension methods - make methods synchronous if they were `async` only for configuration lookups - remove configuration value caching - remove `HttpRequest.IsLocal()` extension method - instead check for development environment - move JSON and XML dependencies into specific receiver projects - make default `WebHookAttribute` constructor `internal` and move its doc comments to `GeneralWebHookAttribute` - add `RouteData.TryGetEventName[s](...)` end `SetWebHookEventNames(...)` extension methods - make most extension names unique to WebHooks - removing `WebHookConfigurationExtensions` entirely is part of #174 nits and smaller TODOs: - correct `IResourceFilter` / `IAsyncResourceFilter` comment inconsistencies - remove unused helper classes - remove `WebHookConstants.ReceiverRouteName` - remove TODO comments for filed issues and already-discussed points - remove comments about the default route name; it's `null` everywhere - remove unused resources - add a few missing `[EditorBrowsable(...)]` attributes on extension classes

dnfclas added the cla-already-signed label Aug 27, 2017

dougbu requested a review from davidfowl August 27, 2017 07:32

dougbu mentioned this pull request Sep 8, 2017

Add ASP.NET Core WebHooks support; one approach to #5 #152

Closed

dougbu changed the base branch from master to dev September 11, 2017 20:13

davidfowl reviewed Sep 13, 2017

View reviewed changes

dougbu force-pushed the dougbu/add.core.II branch 2 times, most recently from 7720ab3 to 79ff808 Compare September 25, 2017 18:55

dougbu force-pushed the dougbu/add.core.II branch from 8c59056 to ce66cd8 Compare October 3, 2017 17:12

dougbu force-pushed the dougbu/add.core.II branch from ce66cd8 to 0c1efa4 Compare October 5, 2017 20:20

dougbu force-pushed the dougbu/add.core.II branch from 0c1efa4 to 9ce1aa4 Compare October 6, 2017 16:44

Eilon mentioned this pull request Oct 6, 2017

Start of an Asp.Net Core port #47

Closed

dougbu force-pushed the dougbu/add.core.II branch from 9ce1aa4 to 2b5429d Compare October 8, 2017 03:12

dougbu changed the base branch from dev to dougbu/stripe.event.data.154 October 8, 2017 03:32

dougbu changed the title ~~Add ASP.NET Core WebHooks support; another approach to #5~~ Add ASP.NET Core WebHooks support (issue #5) Oct 8, 2017

dougbu force-pushed the dougbu/add.core.II branch from 2b5429d to b3783ec Compare October 9, 2017 19:54

dougbu changed the base branch from dougbu/stripe.event.data.154 to dev October 9, 2017 19:55

dougbu added 7 commits October 9, 2017 15:21

Add Microsoft.AspNetCore.WebHooks.Common project

2eedcdb

- other than `Resources` classname, basically unchanged from approach 1

Add Microsoft.AspNetCore.WebHooks.Receivers project

ec28ed7

Move EncodingUtilities to Utilities namespace

ae3661d

Add Microsoft.AspNetCore.WebHooks.Receivers.GitHub project

83b006a

Rearrange Receivers project a bit

d6f26e4

- fill in `WebHookVerifyMethodFilter` - make `[WebHookAction]` a `[consumes]` subclass

Couple 'o bugs

971af68

Name things more consistently, especially ReceiverName

cdf1b6f

dougbu added 5 commits October 16, 2017 16:26

Add more to XyzConstants classes and make names / comments consistent

15dab32

- no more `internal const` fields in Core projects - `WebHookRouteNames` -> `WebHookConstants` and move up to `Microsoft.AspNetCore.WebHooks` namespace

Add rest of Pusher receiver

5384173

Clean up resources

7736517

- remove unused resources - make names and values more consistent

dougbu force-pushed the dougbu/add.core.II branch from b3783ec to 690c6a4 Compare October 16, 2017 23:37

rynowak reviewed Oct 19, 2017

View reviewed changes

rynowak reviewed Oct 23, 2017

View reviewed changes

dougbu commented Oct 28, 2017

View reviewed changes

dougbu closed this Oct 30, 2017

dougbu deleted the dougbu/add.core.II branch October 30, 2017 20:02

dougbu mentioned this pull request Oct 30, 2017

Address some PR comments from #153 #172

Closed

This was referenced Nov 10, 2017

Get rid of TODO and ??? comments #178

Closed

Revamp the IWebHookMetadata registration pattern #179

Closed


		using System.ComponentModel;

		namespace System.Collections.Generic

Add ASP.NET Core WebHooks support (issue #5) #153

Add ASP.NET Core WebHooks support (issue #5) #153

Conversation

dougbu commented Aug 27, 2017

dnfclas commented Aug 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Havret commented Oct 5, 2017

dougbu commented Oct 5, 2017

dougbu commented Oct 5, 2017

Havret commented Oct 5, 2017 • edited Loading

dougbu commented Oct 8, 2017

dougbu commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JDL440 commented Oct 26, 2017

dougbu commented Oct 26, 2017

JDL440 commented Oct 26, 2017

dougbu commented Oct 26, 2017

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Oct 30, 2017

Havret commented Oct 5, 2017 •

edited

Loading