Create Access Key #129

IshamMohamed · 2017-11-01T06:29:13Z

This is the pull request for the "Create Access Key"

masojus · 2017-11-01T06:31:43Z

Thanks, I'll continue review here.

masojus · 2017-11-01T06:37:22Z

I'm not sure why TravisCI succeeded and AppVeyor failed, but are your tests hitting the real server?

IshamMohamed · 2017-11-01T06:40:53Z

Strange, its coming from the last commit - I didn't see a problem in it.

masojus · 2017-11-01T06:51:04Z

The tests don't seem to be mocked, so if it was succeeding before it was because you hadn't switched the keys.

coveralls · 2017-11-01T07:47:49Z

Coverage decreased (-0.8%) to 56.532% when pulling 43ea621 on IshamMohamed:master into b820c2e on keenlabs:master.

coveralls · 2017-11-01T08:52:01Z

Coverage decreased (-0.2%) to 57.117% when pulling 0e2cd7a on IshamMohamed:master into b820c2e on keenlabs:master.

coveralls · 2017-11-01T09:05:45Z

Coverage decreased (-0.1%) to 57.177% when pulling a781c51 on IshamMohamed:master into b820c2e on keenlabs:master.

coveralls · 2017-11-01T09:20:52Z

Coverage decreased (-0.1%) to 57.207% when pulling d2db292 on IshamMohamed:master into b820c2e on keenlabs:master.

masojus · 2017-11-03T06:12:05Z

Keen.NET.Test/KeenClientTest.cs

+            var client = new KeenClient(settings);
+
+            if (UseMocks)
+                client.AccessKeys = new AccessKeysMock(settings,


If the idea is just to do unit testing on the CreateAccessKey() method of the KeenClient, we can just leverage Moq instead of creating a separate class, which might be simpler.

masojus · 2017-11-03T06:13:57Z

Keen/AccessKey/AccessKey.cs

+namespace Keen.Core.AccessKey
+{
+    /// <summary>
+    /// Modal for AccessKey object


Typo: "Modal"->"Model", and we should elaborate on XML doc comments on any public members.

masojus · 2017-11-03T06:15:41Z

Keen/AccessKey/AccessKey.cs

+        public ISet<string> Allowed { get; set; }
+    }
+
+    public class Quaries


Typo: "Quaries"->"Queries"

masojus · 2017-11-03T06:15:51Z

Keen/AccessKey/AccessKey.cs

+        public Writes Writes { get; set; }
+        public Datasets Datasets { get; set; }
+        public CachedQueries CachedQueries { get; set; }
+        public Quaries Queries { get; set; }


Typo: "Quaries"->"Queries"

masojus · 2017-11-03T06:16:24Z

Keen/AccessKey/AccessKey.cs

+
+    }
+
+    public class SavedQueries


All these public types should have XML doc comments.

masojus · 2017-11-03T06:17:59Z

Keen/AccessKey/AccessKeys.cs

+    public class AccessKeys : IAccessKeys
+    {
+        private readonly IKeenHttpClient _keenHttpClient;
+        private readonly string _acceskeyRelativeUrl;


typo: "_acceskey..."->"_accessKey...",

masojus · 2017-11-03T06:23:31Z

Keen/AccessKey/AccessKeys.cs

+            {
+                jsonResponse = JObject.Parse(responseString);
+            }
+            catch (Exception)


Was this because you were seeing a 404 error or something similar and it's actually OK in this case to swallow the exception? Then will we throw below? I know we do this elsewhere, and we should really keep the inner exception around for context, but that's another issue. At least if that's the case, let's put a comment here explaining the empty catch(){} block.

masojus · 2017-11-03T06:25:15Z

Keen/KeenClient.cs

+
+        /// <summary>
+        /// </summary>
+        public void CreateAccessKey(AccessKey.AccessKey accessKey)


Does this need the namespace qualification?

Yes since namespace and class have the same name, even though I used the namespace, C# fails to understand, AccessKey is a class.

If that's the case it seems like we should rename to AccessKeys.

Do you mean the namespace? We have another class called AccessKeys

Yeah, or does that cause other problems?

Yeah in that sense, we may need to go for AccessKeys = new AccessKeys.AccessKeys(_prjSettings, keenHttpClientProvider); in the KeenClient.cs

masojus · 2017-11-03T06:28:10Z

Keen/AccessKey/AccessKey.cs

+        public string Key { get; set; }
+        public Options Options { get; set; }
+
+        internal AccessKey(string name, bool isActive, ISet<string> permitted, Options options)


If the properties all remain public, is it useful to have an internal constructor instead of using object initializer notation and giving the Key auto-prop a default?

masojus · 2017-11-03T06:31:43Z

Keen.NET.Test/KeenClientTest.cs

+
+            HashSet<string> permissions = new HashSet<string>() { "queries" };
+            List<Core.Query.QueryFilter> qFilters = new List<Core.Query.QueryFilter>() { new Core.Query.QueryFilter("customer.id", Core.Query.QueryFilter.FilterOperator.Equals(), "asdf12345z") };
+            CachedQueries cachedQuaries = new CachedQueries();


Typo: "cachedQuaries"->"cachedQueries"

masojus · 2017-11-03T06:33:19Z

Keen/KeenClient.cs

+
+        ///<summary>
+        ///</summary>
+        private async Task<JObject> CreateAccessKeyAsync(AccessKey.AccessKey accessKey)


Does this need the namespace qualification?

masojus · 2017-11-03T06:34:08Z

Keen/AccessKey/IAccessKeys.cs

+
+namespace Keen.Core.AccessKey
+{
+    public interface IAccessKeys


We'll want to flesh out XML doc comments here.

masojus · 2017-11-03T06:36:03Z

Keen/AccessKey/AccessKeys.cs

+using Newtonsoft.Json;
+using Newtonsoft.Json.Linq;
+using Newtonsoft.Json.Serialization;
+using System;


Don't forget to organize and remove unnecessary usings.

masojus · 2017-11-03T06:41:13Z

Keen/AccessKey/IAccessKeys.cs

+        /// </summary>
+        /// <param name="accesskey"></param>
+        /// <returns></returns>
+        Task<JObject> CreateAccessKey(AccessKey accesskey);


We prefer Task-returning methods to use the ...Async(...) naming guideline.

masojus · 2017-11-03T06:43:37Z

Keen.NET.Test/KeenClientTest.cs

@@ -46,6 +47,43 @@ public static void ResetEnv()
    }

    [TestFixture]
+    public class AccessKeysTest : TestBase


I think we should break this out into another file.

masojus · 2017-11-03T06:44:21Z

Keen.NET.Test/KeenClientTest.cs

+                    }));
+
+            HashSet<string> permissions = new HashSet<string>() { "queries" };
+            List<Core.Query.QueryFilter> qFilters = new List<Core.Query.QueryFilter>() { new Core.Query.QueryFilter("customer.id", Core.Query.QueryFilter.FilterOperator.Equals(), "asdf12345z") };


If/when you create a new file for Access Key tests, I'd recommend adding using Keen.Core.Query; and dispensing with the explicit namespace here.

masojus · 2017-11-04T12:38:27Z

Keen/AccessKey/AccessKeys.cs

+                throw new KeenException("An API WriteKey is required to add events.");
+            }
+
+            var xcontent = accesskey.ToString();


What is this xcontent var we have here?

IshamMohamed · 2017-11-07T08:07:58Z

Agreed to all of your changes.

masojus · 2017-11-07T08:13:29Z

@IshamMohamed Are you actively working on this for the rest of the week? What's your ideal timeline for making these suggested revisions? Do you have plans to add the other Access Keys APIs to this, or to leave that for another PR?

IshamMohamed · 2017-11-07T08:17:25Z

Yes I will be. Will finish the suggested revisions before end of this week and start working on the other APIs - but I don't really know if I need another PR for it? do I?

masojus · 2017-11-07T08:20:47Z

Well we'll need to approve and merges changes to master. Speaking of, you'll need to merge from master to get the latest and deal with any potential merge conflicts.

IshamMohamed

Agreed

coveralls · 2017-11-10T03:58:46Z

Coverage decreased (-0.4%) to 69.682% when pulling 7abce95 on IshamMohamed:master into 676e52f on keenlabs:master.

IshamMohamed · 2017-11-10T04:08:44Z

Reviewed all your changed and accepted them. Will work a bit on improving code coverage later today - and come back to some of your questions as well.

masojus · 2017-11-10T16:25:38Z

Ok, if you push some more changes I should be able to review over the weekend. Thanks!

coveralls · 2017-11-11T06:57:31Z

Coverage decreased (-0.6%) to 69.476% when pulling b60d1b1 on IshamMohamed:master into 676e52f on keenlabs:master.

masojus · 2017-11-11T07:21:56Z

@IshamMohamed Let me know when you have something ready for review again. I'd love to get AccessKey creation checked in so we can flesh out the rest of the functionality around AccessKeys.

coveralls · 2017-11-12T14:05:04Z

Coverage decreased (-0.2%) to 72.512% when pulling a7e9a21 on IshamMohamed:master into 536b56b on keenlabs:master.

coveralls · 2017-11-12T14:21:34Z

Coverage decreased (-0.2%) to 72.512% when pulling b9fbe6f on IshamMohamed:master into 536b56b on keenlabs:master.

IshamMohamed · 2017-11-12T14:26:54Z

@masojus can you please let me know, what has to be done to improve the code coverage?

masojus · 2017-11-13T20:55:24Z

A difference of -0.2% isn't a huge deal, but you could look at where code isn't covered and add tests to hit those lines. For example if you look here and here there are some lines that aren't hit, so you could write tests to exercise those setters/getters and hit those error cases.

masojus · 2017-11-14T06:02:38Z

Also, there are some huge changes in master now in preparation for releasing a 0.4.0 .nupkg, but tomorrow I plan on taking these changes and moving them into a branch off of master with all the new stuff and continuing the PR discussion over there...that way I can help deal with all these huge changes and we can get the CreateAccessKey() logic in at least.

masojus · 2017-11-14T07:28:48Z

Okay, I merged master and will push a branch...I'll probably create a new PR tomorrow. You missed several of the comments and changes I requested in this PR, so we'll have to go through and clean those things up. Expect to see those changes tomorrow.

… do, however.

masojus · 2017-11-14T08:36:24Z

Take a look at jm_CreateAccessKey_PR129 to see how things are going with porting this to the new project structure and starting some clean up. I'll make more progress tomorrow and ask for review.

IshamMohamed added 6 commits October 30, 2017 08:26

Added AccessKey class

90b7fc1

Added some changes as per the comments from masojus

32a8394

Added some changes as per the comments from masojus

ac00b1d

Implemented CreateAccessKeys

3596593

Made CreateAccessKeyAsync access modifier to private

1497185

Dummified ProjectSettingsProvider parameters

a7a4b81

Added UseMocks check for CreateAccessKey_Success

43ea621

Added Mock for AccessKeys

0e2cd7a

IshamMohamed added 2 commits November 1, 2017 14:32

Modified CreateAccessKey_Success

08d776d

Removed unnecessary Keen.Core.AccessKey from CreateAccessKey_Success

a781c51

Modified CreateAccessKey_Success

d2db292

masojus reviewed Nov 3, 2017

View reviewed changes

Keen/AccessKey/AccessKey.cs Outdated

public ISet<string> Allowed { get; set; }

}

public class Quaries

Copy link

Contributor

masojus Nov 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "Quaries"->"Queries"

masojus reviewed Nov 3, 2017

View reviewed changes

Keen/AccessKey/AccessKey.cs Outdated

}

public class SavedQueries

Copy link

Contributor

masojus Nov 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these public types should have XML doc comments.

masojus reviewed Nov 3, 2017

View reviewed changes

masojus reviewed Nov 4, 2017

View reviewed changes

IshamMohamed commented Nov 10, 2017

View reviewed changes

Merge branch 'master' into master

7abce95

Changes suggested by "masojus"

b60d1b1

IshamMohamed and others added 3 commits November 12, 2017 19:20

Added a new test case

0e254c6

Merge branch 'master' into master

ffaa48b

Resolved ambiguous reference of dataset

a7e9a21

Extrated AccessKey related tests to a new file.

b9fbe6f

masojus added a commit that referenced this pull request Nov 14, 2017

[WIP] Get a start on cleaning up PR #129. There's still some stuff to…

e1a36d3

… do, however.

Create Access Key #129

Are you sure you want to change the base?

Create Access Key #129

Conversation

IshamMohamed commented Nov 1, 2017

masojus commented Nov 1, 2017

masojus commented Nov 1, 2017 • edited Loading

IshamMohamed commented Nov 1, 2017

masojus commented Nov 1, 2017

coveralls commented Nov 1, 2017

coveralls commented Nov 1, 2017

coveralls commented Nov 1, 2017

coveralls commented Nov 1, 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

masojus Nov 3, 2017 • edited Loading

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

masojus Nov 3, 2017 • edited Loading

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

IshamMohamed commented Nov 7, 2017

masojus commented Nov 7, 2017

IshamMohamed commented Nov 7, 2017

masojus commented Nov 7, 2017

IshamMohamed left a comment

Choose a reason for hiding this comment

coveralls commented Nov 10, 2017

IshamMohamed commented Nov 10, 2017

masojus commented Nov 10, 2017

coveralls commented Nov 11, 2017

masojus commented Nov 11, 2017

coveralls commented Nov 12, 2017

coveralls commented Nov 12, 2017

IshamMohamed commented Nov 12, 2017

masojus commented Nov 13, 2017

masojus commented Nov 14, 2017

masojus commented Nov 14, 2017

masojus commented Nov 14, 2017

masojus commented Nov 1, 2017 •

edited

Loading

masojus Nov 3, 2017 •

edited

Loading

masojus Nov 3, 2017 •

edited

Loading