Add Authentication extra features to Comments tab #41

tttzach · 2020-06-11T23:47:40Z

Additional features added

Show user e-mail when logged in

Associate user e-mail with user comments

…-mail

Accidentally overwrote some new changes

gbuenoandrade · 2020-06-17T19:32:26Z

portfolio/src/main/java/com/google/sps/servlets/AuthenticationServlet.java

+import com.google.gson.Gson;
+import org.javatuples.Pair;
 import com.google.appengine.api.users.UserService;
 import com.google.appengine.api.users.UserServiceFactory;
 import java.io.IOException;


Nit: Imported names should appear in ASCII sort order (https://google.github.io/styleguide/javaguide.html#s3.3.3-import-ordering-and-spacing0

Thanks for pointing this out, I just installed a VSCode extension to sort these.

gbuenoandrade · 2020-06-17T19:34:56Z

portfolio/src/main/java/com/google/sps/servlets/AuthenticationServlet.java

    if (userService.isUserLoggedIn()) {
+      String userEmail = userService.getCurrentUser().getEmail();
      String logoutUrl = userService.createLogoutURL(redirectUrl);
-      response.getWriter().println("<p>Logout <a href=\"" + logoutUrl + "\">here</a>.");
+      Pair<String, String> userInfo = new Pair<>(userEmail, logoutUrl);
+      sendJson(response, userInfo);
    } else {
      String loginUrl = userService.createLoginURL(redirectUrl);
-      response.getWriter().println("<p>Login <a href=\"" + loginUrl + "\">here</a>.");
+      Pair<String, String> userInfo = new Pair<>("N/A", loginUrl);
+      sendJson(response, userInfo);
    }


This could be a good candidate for the return early pattern (https://www.itamarweiss.com/personal/2018/02/28/return-early-pattern.html)

This is an interesting pattern, I've never heard of it. Thanks!

gbuenoandrade · 2020-06-17T19:37:40Z

portfolio/src/main/java/com/google/sps/servlets/AuthenticationServlet.java

+  private void sendJson(HttpServletResponse response, Pair<String, String> userInfo) throws IOException {
+    Gson gson = new Gson();
+    String json = gson.toJson(userInfo);
+    response.getWriter().println(json);
+  }


Nit: If you are using this in more than one class, you could consider creating a utility class to share this logic

@gbuenoandrade I just looked into this, I think this is a great idea. However, one of the sendJson() is defined for Pair and the other is defined for List<String>. Is there a way to add a type-less parameter with two classes that do not share the same abstract class?

gson.toJson accepts Object as parameter, right? Your function could do the same.

I didn't know this, thanks! I've added it!

gbuenoandrade · 2020-06-17T19:38:23Z

portfolio/src/main/java/com/google/sps/servlets/NewCommentServlet.java

+import com.google.appengine.api.users.UserService;
+import com.google.appengine.api.users.UserServiceFactory;
 import com.google.appengine.api.datastore.DatastoreService;
 import com.google.appengine.api.datastore.DatastoreServiceFactory;
 import com.google.appengine.api.datastore.Entity;


Nit: imported names order

Thanks for pointing this out, I just installed a VSCode extension to sort these.

gbuenoandrade · 2020-06-17T19:39:50Z

portfolio/src/main/webapp/libraries.js

+}
+
+function logoutHtml(userEmail, logoutUrl) {
+  return "<p>Hi " + userEmail + "! Logout <a href=\"" + logoutUrl + "\">here</a>.";


Maybe use a template literal?

Good call, thanks!

gbuenoandrade · 2020-06-17T19:43:01Z

portfolio/src/main/webapp/libraries.js

+function getEmail(json) {
+  return json.val0;
+}
+
+function getUrl(json) {
+  return json.val1;
+}


Consider defining your own type to store user info. That way you will avoid having to reference the generic names defined by Pair.

Apologies for the delay, I have implemented this!

gbuenoandrade · 2020-06-17T19:43:18Z

portfolio/src/main/webapp/libraries.js

+}
+
+function loginHtml(loginUrl) {
+  return "<p>Login <a href=\"" + loginUrl + "\">here</a>.";


Maybe use a template literal?

Good call, thanks!

gbuenoandrade · 2020-06-17T19:44:15Z

portfolio/src/main/webapp/libraries.js

+  if (userEmail == 'N/A') {
+    const html = loginHtml(redirectUrl);
+    document.getElementById('login-status').innerHTML = html;
+  } else {
+    const html = logoutHtml(userEmail, redirectUrl);
+    document.getElementById('login-status').innerHTML = html;
+    document.getElementById('comments-form').style.display = "block";
+}


Optional: Maybe return early

I've implemented this as well, thanks!

antmar · 2020-06-22T19:31:05Z

portfolio/src/main/java/com/google/sps/servlets/Pair.java

+
+package com.google.sps.servlets;
+
+public class Pair {


It doesn't really make sense to call this "Pair". A pair is a particular data structure with a generic "first" and "second" member (or something similar).

This is just a specific class for storing login information, and it should just be called LoginInfo or something.

Also, please add javadoc-style comments for the class and all public members, including the constructor.

Yeah, I agree especially now that's a custom class there's no reason to keep the name as Pair. I'll change it!
Ohhh I just found out what javadoc comments are. I deleted them throughout my project because I thought \** looked horrible and didn't add much value. I'll definitely pull a new request for this because I'd have to change this for all the files. #59

RESOLVED #59

antmar · 2020-06-22T19:35:02Z

portfolio/src/main/java/com/google/sps/servlets/AuthenticationServlet.java

    }
+    String loginUrl = userService.createLoginURL(redirectUrl);
+    Pair userInfo = new Pair("N/A", loginUrl);


It's generally a bad idea to use special values like "N/A".

It's easy to make a mistake and forget to check for the special value.

It's easy to change the special value and forget to update all clients.

It's possible that the use of the field will change in the future, and "N/A" may become a possible real value.

It's better to just add another field that holds a boolean to indicate that the user is logged in or not. The documentation on the email field should say that it should be empty when the user is not logged in.

@antmar I agree, having the custom class also makes this easy to do. Just a small consideration, I've altered it such that the constructor has the following function signature: UserInfo(String email, String url, boolean loggedIn). However, if loggedIn = false then I am passing in "" to email. Is there a way to set optional parameters easily like in C++ or Python? Otherwise, would it be wise to overload this function and have one that doesn't require email as a parameter?

Another option is to create separate static factory methods for the loggedIn and loggedOut case.

loggedIn(String email, String url)

loggedOut(String url)

For details about factory methods, see Item #1 in "Effective Java 3rd Ed."

I like this idea, thanks! I've added it. Great reference to the book too.

antmar · 2020-06-22T19:37:12Z

portfolio/src/main/java/com/google/sps/servlets/Pair.java

+
+  Pair(String email, String url) {
+    this.email = email;
+    this.url = url;


"url" is a bit too generic. What does the URL point to? I suggest either:

Use a separate field for login URL and logout URL (only set one of them)

Call this something like "loginOrLogoutUrl".

Good call, thanks!

antmar · 2020-06-22T19:38:06Z

portfolio/src/main/webapp/libraries.js

+
+function getUrl(json) {
+  return json["url"];
+}


I don't think these helpers add any clarity. You can just access the property directly on the json object.

Yeah, I agree. I've removed them, thanks!

antmar · 2020-06-23T20:16:11Z

portfolio/src/main/java/com/google/sps/servlets/JsonUtil.java

+import java.io.IOException;
+import javax.servlet.http.HttpServletResponse;
+
+public final class JsonUtil {


Public classes and methods should have comments.

Thanks! Added the relevant javadoc-style comments for this class. Will go back to the other classes unrelated to this PR here #59

antmar · 2020-06-23T20:24:34Z

portfolio/src/main/java/com/google/sps/servlets/AuthenticationServlet.java

    }
+    String loginUrl = userService.createLoginURL(redirectUrl);
+    Pair userInfo = new Pair("N/A", loginUrl);


Another option is to create separate static factory methods for the loggedIn and loggedOut case.

loggedIn(String email, String url)

loggedOut(String url)

For details about factory methods, see Item #1 in "Effective Java 3rd Ed."

antmar · 2020-06-24T18:30:17Z

portfolio/src/main/java/com/google/sps/servlets/UserInfo.java

+   * @param email
+   * @param logoutUrl
+   */
+  public void loggedIn(String email, String logoutUrl) {


A standard static factory method would have a signature like this:

public static UserInfo loggedIn(String email, String logoutUrl)

The key is that the method would create and initialize the object at the same time. You don't want users of your class to create uninitialized objects. Ideally, you would make the constructor private so that users of the class have to use your factory methods.

Thanks for the clarification - I did not fully understand why the methods had to be static. I have a better idea of what this means now after reviewing your comment and Austin's code on the prototype. I will change this in a new pull request.

RESOLVED #60

tttzach added 2 commits June 11, 2020 19:08

Change /authentication to send JSON instead of HTML to include user e…

1e0f882

…-mail

Show email address of user who posted each comment

7cff3f4

tttzach changed the title ~~Authentication extra features~~ Add Authentication extra features Jun 11, 2020

tttzach changed the title ~~Add Authentication extra features~~ Add Authentication extra features to Comments tab Jun 11, 2020

tttzach changed the base branch from LibrariesAuthentication to master June 17, 2020 19:07

tttzach added 2 commits June 17, 2020 15:09

Merge branch 'master' into LibrariesAuthenticationExtra

0ffb42b

Fix merge conflict commit

0f15f74

Accidentally overwrote some new changes

tttzach requested review from gbuenoandrade and antmar June 17, 2020 19:19

tttzach assigned antmar and gbuenoandrade Jun 17, 2020

gbuenoandrade unassigned antmar Jun 17, 2020

gbuenoandrade reviewed Jun 17, 2020

View reviewed changes

gbuenoandrade assigned tttzach and unassigned gbuenoandrade Jun 17, 2020

tttzach added 2 commits June 18, 2020 14:58

Use return early pattern

0207f37

Resolve merge conflict with master

7c6f54b

tttzach force-pushed the LibrariesAuthenticationExtra branch from cef2430 to 7c6f54b Compare June 18, 2020 19:09

tttzach added 3 commits June 18, 2020 15:21

Use template literals

62c462a

Sort imports

3c02420

Use custom Pair class

7e85c56

tttzach assigned gbuenoandrade and unassigned tttzach Jun 22, 2020

tttzach requested a review from gbuenoandrade June 22, 2020 14:43

tttzach added 2 commits June 22, 2020 12:38

Format according to Codacy suggestions

3670eb7

Format according to Codacy suggestions again

daa4204

gbuenoandrade assigned antmar Jun 22, 2020

antmar reviewed Jun 22, 2020

View reviewed changes

antmar assigned tttzach and unassigned antmar Jun 22, 2020

gbuenoandrade removed their assignment Jun 22, 2020

tttzach added 4 commits June 23, 2020 13:15

Add utility class to centralize sendJson

d1f58d0

Change name of Pair class to UserInfo class

b512044

Store login status as a boolean

55111cf

Remove unnecessary helper functions

316eb68

tttzach requested a review from antmar June 23, 2020 18:41

tttzach assigned antmar and gbuenoandrade and unassigned tttzach Jun 23, 2020

tttzach added 2 commits June 23, 2020 14:43

Reformat according to Codacy suggestions

7b86eaf

Reformat according to Codacy suggestions again

b3b113a

antmar reviewed Jun 23, 2020

View reviewed changes

antmar approved these changes Jun 23, 2020

View reviewed changes

antmar assigned tttzach and unassigned antmar Jun 23, 2020

tttzach added 2 commits June 23, 2020 17:34

Create separate factory methods for loggedIn and loggedOut

84a0c75

Add javadoc-style comments to public class and methods

c858d4f

tttzach merged commit 810c44b into master Jun 23, 2020

antmar reviewed Jun 24, 2020

View reviewed changes

tttzach mentioned this pull request Jul 6, 2020

Fix UserInfo class #60

Merged

Add Authentication extra features to Comments tab #41

Add Authentication extra features to Comments tab #41

Conversation

tttzach commented Jun 11, 2020

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

tttzach Jun 18, 2020 • 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

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

tttzach Jun 23, 2020 • 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

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

tttzach Jun 18, 2020 •

edited

Loading

tttzach Jun 23, 2020 •

edited

Loading