Issue 6: Create an ASP.Net MVC Sample

Issue 6: Create an ASP.Net MVC Sample

[jmcgrew, 2015-01-09 02:56:17]

Hmm, looks like I used the wrong base revision for these diffs...

[jmcgrew, 2015-01-09 18:47:33]

On 2015/01/09 02:56:17, jmcgrew wrote:
> Hmm, looks like I used the wrong base revision for these diffs...

Fixed, PTAL.

(I still need to add a readme file.)

[peleyal, 2015-01-12 08:11:24]

Looks pretty good :)

Some things to consider:
1. I didn't run this sample, just had 10-20 minutes so thought just to take a
look in your code.
2. There are several files that I'm not sure if you changed at all. It will be
great to identify those ones to help the reviewers.
3. I cced google-api-dotnet-client@googlegroups.com and class
4. Will take much longer review after you will work on my first ones.

THANKS SO MUCH!
Eyal

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/BundleConfig.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/BundleConfig.cs:1: using System.Web;
Did you change this file?

If so, add the Google property section.
If not, add: "// This file is added automatically by the Windows Phone basic
application template." As you did before in the Translate sample.

Same in other files as well.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs:4: using System.Web;
empty line between System and Google usings

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs:14: "openid",
Can we get these scopes from one of the generated API, maybe plus?

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs:14: public partial class Startup
again... what did you change in this class?
It will be very helpful to know so the review will be much efficient :)

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Cal...
File Calendar.ASP.NET.MVC5/Calendar.ASP.NET.MVC5.csproj (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Cal...
Calendar.ASP.NET.MVC5/Calendar.ASP.NET.MVC5.csproj:55:
<HintPath>..\packages\Google.Apis.Auth.1.9.0\lib\net40\Google.Apis.Auth.PlatformServices.dll</HintPath>
Please upgrade to 1.9.1 here and there

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
File Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:10: using
Google.Apis.Auth.OAuth2;
1. Google property
2. Separation between System and Google usings

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:33: Scopes =
MyRequestedScopes.GetScopes(),
Why Here it's GetScopes and in MyClientSecerts there are just fields.
I think that I prefer the fields case

BUT anyway just be consistent.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:64: var success = await
credential.RefreshTokenAsync(CancellationToken.None);
The library is suppose to that for you, you don't need to ask to refresh token,
it's the library responsibility to check before a request is made if the access
token is still valid..

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:88: var service = new
CalendarService(initializer);
Just nicer to write the following in my opinion,

var initializer = new BaseClientService.Initializer()
{
    HttpClientInitializer = credential,
    ApplicationName = "ASP.NET MVC5 Calendar Sample",
}
var service = new CalendarService(initializer);

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:103: var fetchResults =
await Task.WhenAll(fetchTasks);
sweet :)

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Mod...
File Calendar.ASP.NET.MVC5/Models/CalendarEventGroup.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Mod...
Calendar.ASP.NET.MVC5/Models/CalendarEventGroup.cs:9: {
Please add comments to class and properties

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Mod...
File Calendar.ASP.NET.MVC5/Models/IdentityModels.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Mod...
Calendar.ASP.NET.MVC5/Models/IdentityModels.cs:9: // You can add profile data
for the user by adding more properties to your ApplicationUser class, please
visit http://go.microsoft.com/fwlink/?LinkID=317594 to learn more.
Please keep no more than 120 chars per lijne

[jmcgrew, 2015-02-20 02:53:17]

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/BundleConfig.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/BundleConfig.cs:1: using System.Web;
On 2015/01/12 08:11:24, peleyal wrote:
> Did you change this file?
> 
> If so, add the Google property section.
> If not, add: "// This file is added automatically by the Windows Phone basic
> application template." As you did before in the Translate sample.
> 
> Same in other files as well.

Done.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs:4: using System.Web;
On 2015/01/12 08:11:24, peleyal wrote:
> empty line between System and Google usings

Done.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs:14: "openid",
On 2015/01/12 08:11:24, peleyal wrote:
> Can we get these scopes from one of the generated API, maybe plus?

I can use PlusService.Scope.PlusLogin and .UserinfoEmail, which are similar but
not exactly the same: https://developers.google.com/+/api/oauth#scopes

In the auth UI they appear to be requesting some scary permissions:
"Know your basic profile info and list of people in your circles. *Includes
people in circles that are not public on your profile.*"
"Allow Google to let the people in these circles know that you have signed in to
this app with Google: [Your circles]"

Also, "openid" and "email" are standard scopes in the OpenID Connect spec, so it
seems a little wasteful to add a dependency on Google.Apis.Plus.v1 just for
this. Thoughts?

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs:14: public partial class Startup
On 2015/01/12 08:11:24, peleyal wrote:
> again... what did you change in this class?
> It will be very helpful to know so the review will be much efficient :)

Done.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Cal...
File Calendar.ASP.NET.MVC5/Calendar.ASP.NET.MVC5.csproj (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Cal...
Calendar.ASP.NET.MVC5/Calendar.ASP.NET.MVC5.csproj:55:
<HintPath>..\packages\Google.Apis.Auth.1.9.0\lib\net40\Google.Apis.Auth.PlatformServices.dll</HintPath>
On 2015/01/12 08:11:24, peleyal wrote:
> Please upgrade to 1.9.1 here and there

Updated all NuGet packages.

Is there a new version of Google.Apis.Calendar.v3? I'm still only seeing 1.9.0.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
File Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:10: using
Google.Apis.Auth.OAuth2;
On 2015/01/12 08:11:23, peleyal wrote:
> 1. Google property
> 2. Separation between System and Google usings

Done. Is that right: System, 3rd party (Microsoft but not built into .NET),
Google, project specific?

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:33: Scopes =
MyRequestedScopes.GetScopes(),
On 2015/01/12 08:11:24, peleyal wrote:
> Why Here it's GetScopes and in MyClientSecerts there are just fields.
> I think that I prefer the fields case
> 
> BUT anyway just be consistent.

Well, I didn't want to use a field because it's a mutable array, and this way I
return a fresh array with the right contents every time... how about a property
so the syntax is consistent? (See change in MyRequestedScope.cs.)

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:64: var success = await
credential.RefreshTokenAsync(CancellationToken.None);
On 2015/01/12 08:11:24, peleyal wrote:
> The library is suppose to that for you, you don't need to ask to refresh
token,
> it's the library responsibility to check before a request is made if the
access
> token is still valid..

Done.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:88: var service = new
CalendarService(initializer);
On 2015/01/12 08:11:24, peleyal wrote:
> Just nicer to write the following in my opinion,
> 
> var initializer = new BaseClientService.Initializer()
> {
>     HttpClientInitializer = credential,
>     ApplicationName = "ASP.NET MVC5 Calendar Sample",
> }
> var service = new CalendarService(initializer);

Done.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Mod...
File Calendar.ASP.NET.MVC5/Models/CalendarEventGroup.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Mod...
Calendar.ASP.NET.MVC5/Models/CalendarEventGroup.cs:9: {
On 2015/01/12 08:11:24, peleyal wrote:
> Please add comments to class and properties

Done.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Mod...
File Calendar.ASP.NET.MVC5/Models/IdentityModels.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Mod...
Calendar.ASP.NET.MVC5/Models/IdentityModels.cs:9: // You can add profile data
for the user by adding more properties to your ApplicationUser class, please
visit http://go.microsoft.com/fwlink/?LinkID=317594 to learn more.
On 2015/01/12 08:11:23, peleyal wrote:
> Please keep no more than 120 chars per lijne

Done.

[class, 2015-02-23 17:08:35]

Added a few notes on outstanding comments.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs:14: "openid",
On 2015/02/20 02:53:17, jmcgrew wrote:
> On 2015/01/12 08:11:24, peleyal wrote:
> > Can we get these scopes from one of the generated API, maybe plus?
> 
> I can use PlusService.Scope.PlusLogin and .UserinfoEmail, which are similar
but
> not exactly the same: https://developers.google.com/+/api/oauth#scopes
> 
> In the auth UI they appear to be requesting some scary permissions:
> "Know your basic profile info and list of people in your circles. *Includes
> people in circles that are not public on your profile.*"
> "Allow Google to let the people in these circles know that you have signed in
to
> this app with Google: [Your circles]"
> 
> Also, "openid" and "email" are standard scopes in the OpenID Connect spec, so
it
> seems a little wasteful to add a dependency on Google.Apis.Plus.v1 just for
> this. Thoughts?

Using the raw "email" scope string seems fine.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs:9: using System.Security.Claims;
Personally, I group together System.*, 1P.*, 3P.* and place newlines between
groups

using System;
using System.Security.Claims;
using System.Threading.Tasks.

using Calendar.ASP.NET.MVC5.Models;

using Microsoft.AspNet.Identity;
using Microsoft.AspNet.Identity.Owin;
using Microsoft.Owin;
using Microsoft.Owin.Security.Cookies;
using Microsoft.Owin.Security.Google;
using Owin;

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Cal...
File Calendar.ASP.NET.MVC5/Calendar.ASP.NET.MVC5.csproj (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Cal...
Calendar.ASP.NET.MVC5/Calendar.ASP.NET.MVC5.csproj:55:
<HintPath>..\packages\Google.Apis.Auth.1.9.0\lib\net40\Google.Apis.Auth.PlatformServices.dll</HintPath>
On 2015/02/20 02:53:17, jmcgrew wrote:
> On 2015/01/12 08:11:24, peleyal wrote:
> > Please upgrade to 1.9.1 here and there
> 
> Updated all NuGet packages.
> 
> Is there a new version of Google.Apis.Calendar.v3? I'm still only seeing
1.9.0.

Looks like it's still 1.9.0

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
File Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:10: using
Google.Apis.Auth.OAuth2;
On 2015/02/20 02:53:17, jmcgrew wrote:
> On 2015/01/12 08:11:23, peleyal wrote:
> > 1. Google property
> > 2. Separation between System and Google usings
> 
> Done. Is that right: System, 3rd party (Microsoft but not built into .NET),
> Google, project specific?

I have usually done 
* System
* Google
* 3P (e.g. Microsoft.* Newtonsoft.*)

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/MyClientSecrets.cs (right):

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/MyClientSecrets.cs:25: /// Holds the Google API
client secrets. Replace the values below with credentials from your developer
console.
(Optional) Add a link to the developer console:

https://console.developers.google.com

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs (right):

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs:33: "openid",
Note that OpenID is being deprecated in April:

https://developers.google.com/+/api/auth-migration

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Pro...
File Calendar.ASP.NET.MVC5/Properties/AssemblyInfo.cs (right):

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Pro...
Calendar.ASP.NET.MVC5/Properties/AssemblyInfo.cs:1: using System.Reflection;
Add comment indicating this was autogenerated or add the apache 2.0 header.

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Scr...
File Calendar.ASP.NET.MVC5/Scripts/bootstrap.js (right):

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Scr...
Calendar.ASP.NET.MVC5/Scripts/bootstrap.js:2: * Bootstrap v3.3.2
(http://getbootstrap.com)
I'm not sure if it's possible / practical, but consider removing bootstrap
sources from the repository and having them get fulfilled by NuGet.

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Scr...
File Calendar.ASP.NET.MVC5/Scripts/jquery-2.1.3.js (right):

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Scr...
Calendar.ASP.NET.MVC5/Scripts/jquery-2.1.3.js:2: * jQuery JavaScript Library
v2.1.3
Same with jQuery, if possible, use the CDN-hosted (Google-hosted) jQuery or let
NuGet satisfy the jQuery dependency.

[jmcgrew, 2015-02-28 01:33:41]

This addresses most of the 2nd round of notes - I still need to look into
removing the 3rd party dependencies in favor of NuGet or CDN.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs:9: using System.Security.Claims;
On 2015/02/23 17:08:34, class wrote:
> Personally, I group together System.*, 1P.*, 3P.* and place newlines between
> groups
> 
> using System;
> using System.Security.Claims;
> using System.Threading.Tasks.
> 
> using Calendar.ASP.NET.MVC5.Models;
> 
> using Microsoft.AspNet.Identity;
> using Microsoft.AspNet.Identity.Owin;
> using Microsoft.Owin;
> using Microsoft.Owin.Security.Cookies;
> using Microsoft.Owin.Security.Google;
> using Owin;

Done.

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
File Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs (right):

https://codereview.appspot.com/194980043/diff/10001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:10: using
Google.Apis.Auth.OAuth2;
On 2015/02/23 17:08:34, class wrote:
> On 2015/02/20 02:53:17, jmcgrew wrote:
> > On 2015/01/12 08:11:23, peleyal wrote:
> > > 1. Google property
> > > 2. Separation between System and Google usings
> > 
> > Done. Is that right: System, 3rd party (Microsoft but not built into .NET),
> > Google, project specific?
> 
> I have usually done 
> * System
> * Google
> * 3P (e.g. Microsoft.* Newtonsoft.*)

Done.

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/MyClientSecrets.cs (right):

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/MyClientSecrets.cs:25: /// Holds the Google API
client secrets. Replace the values below with credentials from your developer
console.
On 2015/02/23 17:08:35, class wrote:
> (Optional) Add a link to the developer console:
> 
> https://console.developers.google.com

Done.

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs (right):

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/MyRequestedScopes.cs:33: "openid",
On 2015/02/23 17:08:35, class wrote:
> Note that OpenID is being deprecated in April:
> 
> https://developers.google.com/+/api/auth-migration

Indeed - the "openid" scope is for the new system, OpenID Connect, which is
based on OAuth 2.0.

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Pro...
File Calendar.ASP.NET.MVC5/Properties/AssemblyInfo.cs (right):

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Pro...
Calendar.ASP.NET.MVC5/Properties/AssemblyInfo.cs:1: using System.Reflection;
On 2015/02/23 17:08:35, class wrote:
> Add comment indicating this was autogenerated or add the apache 2.0 header.

Done.

[jmcgrew, 2015-03-03 00:41:16]

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Scr...
File Calendar.ASP.NET.MVC5/Scripts/jquery-2.1.3.js (right):

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Scr...
Calendar.ASP.NET.MVC5/Scripts/jquery-2.1.3.js:2: * jQuery JavaScript Library
v2.1.3
On 2015/02/23 17:08:35, class wrote:
> Same with jQuery, if possible, use the CDN-hosted (Google-hosted) jQuery or
let
> NuGet satisfy the jQuery dependency.

Per
http://stackoverflow.com/questions/19369766/how-do-i-reference-instead-of-cop...,
this doesn't work with NuGet: package restore won't copy files into the content
folders.

ASP.NET can be configured to use a CDN for scripts
(http://www.asp.net/mvc/overview/performance/bundling-and-minification), but
they recommend keeping a local copy as a fallback anyway, which requires some
extra JS code as well as checking in the files. I think maybe we should keep the
dependencies as-is for now.

[class, 2015-03-03 19:39:37]

I'd consider the CDN-hosting of libraries a nit at this point. LGTM.

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Scr...
File Calendar.ASP.NET.MVC5/Scripts/jquery-2.1.3.js (right):

https://codereview.appspot.com/194980043/diff/20001/Calendar.ASP.NET.MVC5/Scr...
Calendar.ASP.NET.MVC5/Scripts/jquery-2.1.3.js:2: * jQuery JavaScript Library
v2.1.3
> maybe we should keep the
> dependencies as-is for now.

SGTM. If we want to move to CDN-hosted, there is MaxCDN for bootstrap -
http://getbootstrap.com/getting-started/ and Google CDN for jQuery -
https://developers.google.com/speed/libraries/

[peleyal, 2015-03-04 15:08:14]

I'll take a brief look later on this week, I prefer to take another look,
so it will be great if you wait until Monday.

Thanks and sorry,
Eyal


On Tue, Mar 3, 2015 at 2:39 PM <class@google.com> wrote:

> I'd consider the CDN-hosting of libraries a nit at this point. LGTM.
>
>
> https://codereview.appspot.com/194980043/diff/20001/
> Calendar.ASP.NET.MVC5/Scripts/jquery-2.1.3.js
> File Calendar.ASP.NET.MVC5/Scripts/jquery-2.1.3.js (right):
>
> https://codereview.appspot.com/194980043/diff/20001/
> Calendar.ASP.NET.MVC5/Scripts/jquery-2.1.3.js#newcode2
> Calendar.ASP.NET.MVC5/Scripts/jquery-2.1.3.js:2: * jQuery JavaScript
> Library v2.1.3
> > maybe we should keep the
> > dependencies as-is for now.
>
> SGTM. If we want to move to CDN-hosted, there is MaxCDN for bootstrap -
> http://getbootstrap.com/getting-started/ and Google CDN for jQuery -
> https://developers.google.com/speed/libraries/
>
> https://codereview.appspot.com/194980043/
>

[peleyal, 2015-03-07 00:17:44]

I'm not an ASP.NET MVC expert, so it is a little bit hard to understand some of
the code, like the AuthenticationManager parts. I'll be happy to get a better
review, but I swapped :(

So, hopefully I'll have more time after the perf season and we can set up a
meeting to chat and run the sample.

Thanks so much for doing so, I added small nits mostly of usings order.

Eyal

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs:32: using
Microsoft.Owin.Security;
Please add an empty line

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs:103: var googleRefreshToken =
externalIdentity.FindFirst(MyClaimTypes.GoogleRefreshToken);
Why do we need to save the access and refresh token here? They should be stored
in the auth library (using the data store)? It looks like I'm missing something

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs:25: using
Calendar.ASP.NET.MVC5.Models;
Calendar and Google APIs should be in the end. And I'll put System and Microsoft
together.
That should be the order:


system/microsoft

3rd parties

yours\google

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
File Calendar.ASP.NET.MVC5/Controllers/AccountController.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/AccountController.cs:22: using
Microsoft.Owin.Security;
empty line please

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/AccountController.cs:47: return _signInManager
?? HttpContext.GetOwinContext().Get<ApplicationSignInManager>();
Do you want to cache it for the next time, something like:
if (_signInManager == null)
{
    _signInManager =
HttpContext.GetOwinContext().Get<ApplicationSignInManager>();
}
return _signInManager;

Same below

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
File Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:25: using
Calendar.ASP.NET.MVC5.Models;
same

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:105: var fetchResults =
await Task.WhenAll(fetchTasks);
Nice :)

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
File Calendar.ASP.NET.MVC5/Controllers/HomeController.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/HomeController.cs:3: 
Did you change this file? If now please add the other header that you use for
untouched files.

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Mig...
File Calendar.ASP.NET.MVC5/Migrations/201411270135001_Init.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Mig...
Calendar.ASP.NET.MVC5/Migrations/201411270135001_Init.cs:1: // This file is
added automatically by the MVC 5 application template.
Seriously? Crap :(

[jmcgrew, 2015-04-06 23:01:52]

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs:32: using
Microsoft.Owin.Security;
On 2015/03/07 00:17:43, peleyal wrote:
> Please add an empty line 

Done.

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs:103: var googleRefreshToken =
externalIdentity.FindFirst(MyClaimTypes.GoogleRefreshToken);
On 2015/03/07 00:17:43, peleyal wrote:
> Why do we need to save the access and refresh token here? They should be
stored
> in the auth library (using the data store)? It looks like I'm missing
something

These tokens are returned in the OAuth login response to the MVC third-party
login provider. At Startup.Auth.cs:86, we copy them into a temporary identity,
and then here we copy them from the temporary identity to the real identity
where they'll be stored alongside the rest of the user's MVC profile. Is there
an advantage to storing them in the API client's data store instead?

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs:25: using
Calendar.ASP.NET.MVC5.Models;
On 2015/03/07 00:17:43, peleyal wrote:
> Calendar and Google APIs should be in the end. And I'll put System and
Microsoft
> together.
> That should be the order:
> 
> 
> system/microsoft
> 
> 3rd parties
> 
> yours\google

Done.

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
File Calendar.ASP.NET.MVC5/Controllers/AccountController.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/AccountController.cs:22: using
Microsoft.Owin.Security;
On 2015/03/07 00:17:43, peleyal wrote:
> empty line please

Done.

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/AccountController.cs:47: return _signInManager
?? HttpContext.GetOwinContext().Get<ApplicationSignInManager>();
On 2015/03/07 00:17:43, peleyal wrote:
> Do you want to cache it for the next time, something like:
> if (_signInManager == null)
> {
>     _signInManager =
> HttpContext.GetOwinContext().Get<ApplicationSignInManager>();
> }
> return _signInManager;
> 
> Same below

This file is actually part of the application template - I've updated the
comment at the top. I suspect they may have a reason for not caching the value
here, e.g. so it can be changed on the Owin context.

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
File Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs:25: using
Calendar.ASP.NET.MVC5.Models;
On 2015/03/07 00:17:43, peleyal wrote:
> same

Done.

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
File Calendar.ASP.NET.MVC5/Controllers/HomeController.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Con...
Calendar.ASP.NET.MVC5/Controllers/HomeController.cs:3: 
On 2015/03/07 00:17:43, peleyal wrote:
> Did you change this file? If now please add the other header that you use for
> untouched files.

Good catch, done.

[peleyal, 2015-04-08 13:36:20]

Sorry it takes forever :)
Just I still didn't get if the client library can already store your credential
(like we discussed offline).

Besides that LGTM

Can you also attach some screenshots of screen to CL description?

Thanks!

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
File Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs:103: var googleRefreshToken =
externalIdentity.FindFirst(MyClaimTypes.GoogleRefreshToken);
On 2015/04/06 23:01:52, jmcgrew wrote:
> On 2015/03/07 00:17:43, peleyal wrote:
> > Why do we need to save the access and refresh token here? They should be
> stored
> > in the auth library (using the data store)? It looks like I'm missing
> something
> 
> These tokens are returned in the OAuth login response to the MVC third-party
> login provider. At Startup.Auth.cs:86, we copy them into a temporary identity,
> and then here we copy them from the temporary identity to the real identity
> where they'll be stored alongside the rest of the user's MVC profile. Is there
> an advantage to storing them in the API client's data store instead?

It just that the client library already stored all those values (except user id)
for you, so why do we need to store them in two different places?

[LindaL, 2015-06-26 08:21:10]

On 2015/04/08 13:36:20, peleyal wrote:
> Sorry it takes forever :)
> Just I still didn't get if the client library can already store your
credential
> (like we discussed offline).
> 
> Besides that LGTM
> 
> Can you also attach some screenshots of screen to CL description?
> 
> Thanks!
> 
>
https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
> File Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs (right):
> 
>
https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
> Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs:103: var googleRefreshToken
=
> externalIdentity.FindFirst(MyClaimTypes.GoogleRefreshToken);
> On 2015/04/06 23:01:52, jmcgrew wrote:
> > On 2015/03/07 00:17:43, peleyal wrote:
> > > Why do we need to save the access and refresh token here? They should be
> > stored
> > > in the auth library (using the data store)? It looks like I'm missing
> > something
> > 
> > These tokens are returned in the OAuth login response to the MVC third-party
> > login provider. At Startup.Auth.cs:86, we copy them into a temporary
identity,
> > and then here we copy them from the temporary identity to the real identity
> > where they'll be stored alongside the rest of the user's MVC profile. Is
there
> > an advantage to storing them in the API client's data store instead?
> 
> It just that the client library already stored all those values (except user
id)
> for you, so why do we need to store them in two different places?

Was this released?  I have someone on my website looking for some help with MVC
/ owin / client lib mix authentication.

[jmcgrew, 2015-06-26 18:33:54]

On 2015/06/26 08:21:10, LindaL wrote:
> On 2015/04/08 13:36:20, peleyal wrote:
> > Sorry it takes forever :)
> > Just I still didn't get if the client library can already store your
> credential
> > (like we discussed offline).
> > 
> > Besides that LGTM
> > 
> > Can you also attach some screenshots of screen to CL description?
> > 
> > Thanks!
> > 
> >
>
https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
> > File Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs (right):
> > 
> >
>
https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
> > Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs:103: var
googleRefreshToken
> =
> > externalIdentity.FindFirst(MyClaimTypes.GoogleRefreshToken);
> > On 2015/04/06 23:01:52, jmcgrew wrote:
> > > On 2015/03/07 00:17:43, peleyal wrote:
> > > > Why do we need to save the access and refresh token here? They should be
> > > stored
> > > > in the auth library (using the data store)? It looks like I'm missing
> > > something
> > > 
> > > These tokens are returned in the OAuth login response to the MVC
third-party
> > > login provider. At Startup.Auth.cs:86, we copy them into a temporary
> identity,
> > > and then here we copy them from the temporary identity to the real
identity
> > > where they'll be stored alongside the rest of the user's MVC profile. Is
> there
> > > an advantage to storing them in the API client's data store instead?
> > 
> > It just that the client library already stored all those values (except user
> id)
> > for you, so why do we need to store them in two different places?
> 
> Was this released?  I have someone on my website looking for some help with
MVC
> / owin / client lib mix authentication.

It hasn't been released yet - the migration to Github caught me at a busy time
so I haven't had much of a chance to push it through the new release process.
I've added it to my todo list for next week.

[peleyal, 2015-06-26 19:11:30]

Be aware that I'm going to release a new library in two weeks from now...
so if you are going to push it next week it will be awesome!

On Fri, Jun 26, 2015 at 2:33 PM <jmcgrew@google.com> wrote:

> On 2015/06/26 08:21:10, LindaL wrote:
> > On 2015/04/08 13:36:20, peleyal wrote:
> > > Sorry it takes forever :)
> > > Just I still didn't get if the client library can already store your
> > credential
> > > (like we discussed offline).
> > >
> > > Besides that LGTM
> > >
> > > Can you also attach some screenshots of screen to CL description?
> > >
> > > Thanks!
> > >
> > >
>
>
>
https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
> > > File Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs (right):
> > >
> > >
>
>
>
https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
> > > Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs:103: var
> googleRefreshToken
> > =
> > > externalIdentity.FindFirst(MyClaimTypes.GoogleRefreshToken);
> > > On 2015/04/06 23:01:52, jmcgrew wrote:
> > > > On 2015/03/07 00:17:43, peleyal wrote:
> > > > > Why do we need to save the access and refresh token here? They
> should be
> > > > stored
> > > > > in the auth library (using the data store)? It looks like I'm
> missing
> > > > something
> > > >
> > > > These tokens are returned in the OAuth login response to the MVC
> third-party
> > > > login provider. At Startup.Auth.cs:86, we copy them into a
> temporary
> > identity,
> > > > and then here we copy them from the temporary identity to the real
> identity
> > > > where they'll be stored alongside the rest of the user's MVC
> profile. Is
> > there
> > > > an advantage to storing them in the API client's data store
> instead?
> > >
> > > It just that the client library already stored all those values
> (except user
> > id)
> > > for you, so why do we need to store them in two different places?
>
> > Was this released?  I have someone on my website looking for some help
> with MVC
> > / owin / client lib mix authentication.
>
> It hasn't been released yet - the migration to Github caught me at a
> busy time so I haven't had much of a chance to push it through the new
> release process. I've added it to my todo list for next week.
>
> https://codereview.appspot.com/194980043/
>

[LindaL, 2015-06-29 09:12:52]

I sent the link to someone anyway,  it helped him fix his problem.
Thought you would like to know that :)

I am still MVC nub so pointing him at your tutorial worked.

Linda

On Fri, Jun 26, 2015 at 9:11 PM, Eyal Peled <peleyal@google.com> wrote:

> Be aware that I'm going to release a new library in two weeks from now...
> so if you are going to push it next week it will be awesome!
>
> On Fri, Jun 26, 2015 at 2:33 PM <jmcgrew@google.com> wrote:
>
>> On 2015/06/26 08:21:10, LindaL wrote:
>> > On 2015/04/08 13:36:20, peleyal wrote:
>> > > Sorry it takes forever :)
>> > > Just I still didn't get if the client library can already store your
>> > credential
>> > > (like we discussed offline).
>> > >
>> > > Besides that LGTM
>> > >
>> > > Can you also attach some screenshots of screen to CL description?
>> > >
>> > > Thanks!
>> > >
>> > >
>>
>>
>>
https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
>> > > File Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs (right):
>> > >
>> > >
>>
>>
>>
https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App...
>> > > Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs:103: var
>> googleRefreshToken
>> > =
>> > > externalIdentity.FindFirst(MyClaimTypes.GoogleRefreshToken);
>> > > On 2015/04/06 23:01:52, jmcgrew wrote:
>> > > > On 2015/03/07 00:17:43, peleyal wrote:
>> > > > > Why do we need to save the access and refresh token here? They
>> should be
>> > > > stored
>> > > > > in the auth library (using the data store)? It looks like I'm
>> missing
>> > > > something
>> > > >
>> > > > These tokens are returned in the OAuth login response to the MVC
>> third-party
>> > > > login provider. At Startup.Auth.cs:86, we copy them into a
>> temporary
>> > identity,
>> > > > and then here we copy them from the temporary identity to the real
>> identity
>> > > > where they'll be stored alongside the rest of the user's MVC
>> profile. Is
>> > there
>> > > > an advantage to storing them in the API client's data store
>> instead?
>> > >
>> > > It just that the client library already stored all those values
>> (except user
>> > id)
>> > > for you, so why do we need to store them in two different places?
>>
>> > Was this released?  I have someone on my website looking for some help
>> with MVC
>> > / owin / client lib mix authentication.
>>
>> It hasn't been released yet - the migration to Github caught me at a
>> busy time so I haven't had much of a chance to push it through the new
>> release process. I've added it to my todo list for next week.
>>
>> https://codereview.appspot.com/194980043/
>>
>


-- 
Linda Lawton

Google Developer Expert for Google Analytics
<https://developers.google.com/experts/people/linda-lawton>
Blog - Daimto.com <http://www.daimto.com/>
Google+ - Linda Lawton  <https://plus.google.com/u/0/+LindaLawton>