Re: Issue 6: Create an ASP.Net MVC Sample (issue 194980043 by jmcgrew@google.com)

[Eyal Peled]

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 <cl...@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/

[pel...@google.com]

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_Start/IdentityConfig.cs
File Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App_Start/IdentityConfig.cs#newcode32
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_Start/IdentityConfig.cs#newcode103
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_Start/Startup.Auth.cs
File Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/App_Start/Startup.Auth.cs#newcode25
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/Controllers/AccountController.cs
File Calendar.ASP.NET.MVC5/Controllers/AccountController.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Controllers/AccountController.cs#newcode22
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/Controllers/AccountController.cs#newcode47
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/Controllers/CalendarController.cs
File Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Controllers/CalendarController.cs#newcode25
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/Controllers/CalendarController.cs#newcode105
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/Controllers/HomeController.cs
File Calendar.ASP.NET.MVC5/Controllers/HomeController.cs (right):

https://codereview.appspot.com/194980043/diff/30001/Calendar.ASP.NET.MVC5/Controllers/HomeController.cs#newcode3
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/Migrations/201411270135001_Init.cs
File Calendar.ASP.NET.MVC5/Migrations/201411270135001_Init.cs (right):

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

https://codereview.appspot.com/194980043/