Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Authentication extra features to Comments tab #41

Merged
merged 19 commits into from
Jun 23, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions portfolio/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@
<artifactId>appengine-api-1.0-sdk</artifactId>
<version>1.9.59</version>
</dependency>
<dependency>
<groupId>org.javatuples</groupId>
<artifactId>javatuples</artifactId>
<version>1.2</version>
</dependency>
<dependency>
<groupId>com.opencsv</groupId>
<artifactId>opencsv</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.sps.servlets;

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

RESOLVED

Expand All @@ -27,16 +29,25 @@ public class AuthenticationServlet extends HttpServlet {

@Override
public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException {
response.setContentType("text/html");
response.setContentType("application/json");
UserService userService = UserServiceFactory.getUserService();
String redirectUrl = "/index.html";
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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

RESOLVED

}

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

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

@tttzach tttzach Jun 18, 2020

Choose a reason for hiding this comment

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

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

RESOLVED


}
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@ private List<String> getCommentsToDisplay(PreparedQuery results, int max) {
Iterator<Entity> iterator = results.asIterator();
for (int i = 0; (i < max) && (iterator.hasNext()); ++i) {
Entity entity = iterator.next();
String email = (String) entity.getProperty("email");
String comment = (String) entity.getProperty("comment");
comments.add(comment);
comments.add(email + ": " + comment);
}
return comments;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.sps.servlets;

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: imported names order

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

RESOLVED

Expand All @@ -29,17 +31,20 @@ public class NewCommentServlet extends HttpServlet {

@Override
public void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException {
UserService userService = UserServiceFactory.getUserService();
String email = userService.getCurrentUser().getEmail();
String comment = request.getParameter("comment");
long timestamp = System.currentTimeMillis();
Entity commentEntity = createEntity(comment, timestamp);
Entity commentEntity = createEntity(comment, timestamp, email);
putEntity(commentEntity);
response.sendRedirect("/index.html");
}

private Entity createEntity(String comment, long timestamp) {
private Entity createEntity(String comment, long timestamp, String email) {
Entity entity = new Entity("Comment");
entity.setProperty("comment", comment);
entity.setProperty("timestamp", timestamp);
entity.setProperty("email", email);
return entity;
}

Expand Down
31 changes: 26 additions & 5 deletions portfolio/src/main/webapp/libraries.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,30 @@ function drawCoronavirusChart() {

async function getLoginStatus() {
const response = await fetch('/authentication');
const responseHtml = await response.text();
document.getElementById('login-status').innerHTML = responseHtml;
if (responseHtml.includes('Logout')) {
document.getElementById('comments-form').style.display = 'block';
}
const json = await response.json();
const userEmail = getEmail(json);
const redirectUrl = getUrl(json);
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";
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: Maybe return early

Copy link
Owner Author

Choose a reason for hiding this comment

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

I've implemented this as well, thanks!

Copy link
Owner Author

Choose a reason for hiding this comment

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

RESOLVED


function getEmail(json) {
return json.val0;
}

function getUrl(json) {
return json.val1;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Apologies for the delay, I have implemented this!

Copy link
Owner Author

Choose a reason for hiding this comment

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

RESOLVED

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

RESOLVED


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

Choose a reason for hiding this comment

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

Maybe use a template literal?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good call, thanks!

Copy link
Owner Author

Choose a reason for hiding this comment

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

RESOLVED

}

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

Choose a reason for hiding this comment

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

Maybe use a template literal?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good call, thanks!

Copy link
Owner Author

Choose a reason for hiding this comment

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

RESOLVED

}