Skip to content
This repository has been archived by the owner on Aug 14, 2020. It is now read-only.

Pramith/backend routes #61

Merged
merged 8 commits into from
May 20, 2020
Merged

Pramith/backend routes #61

merged 8 commits into from
May 20, 2020

Conversation

pramithready
Copy link
Collaborator

added user likes stuff and user/courseID routes

Copy link
Collaborator

@daniel-d-truong daniel-d-truong left a comment

Choose a reason for hiding this comment

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

looks good Pramith! just a couple of minor changes that i'd like, let me know if you have any questions about them!

addLikedPost = async (postId) => {
this.props.likedPostList.push(postId);
let post = await getPostById(postId);
post.incrementScore(); // await?
Copy link
Collaborator

Choose a reason for hiding this comment

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

await shouldn't be necessary UNLESS you want to wait for the "this.push()" to finish being executed in the incrementScore method. I don't think it'll be necessary though since it'll be faster if we let the two updates to Firebase happen at the same time (feel free to delete the comment as well)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated to implementation but what do you think about having each post collect the users that like a post (I believe we could do this implementation wise by adding a user_list to post) but do you think we should do that or is it better to make likes anonymous (maybe we can ask the team about this too)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think collecting the users that like a post would be a good idea. It's easy to implement and offers us more options in the future so I think it's a good idea

@@ -17,7 +17,7 @@ class Course {
}

getInstructorList() {
return this.props.instructorList;
return this.props.instructorList.slice(1, this.props.instructorList.length);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the purpose of this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rohith said that he and Pawan were going to initialize each list with a dummy string element to make sure Javascript knew that the list was a list of Strings. They wanted the list getters implemented this way to ignore that first dummy string

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh interesting okay I must not have caught that in a previous PR. Honestly that shouldn't be necessary but that could also be a refactor for later. Let's make a comment or something about that in the code to prevent future confusion.

@@ -35,6 +35,34 @@ class User {
await this.push();
}

addLikedPost = async (postId) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In a future PR let's write tests for these to make sure that they work

const courseObj = await course.getCourseById(courseUUID);

if (courseObj.getInstructorList().indexOf(userObj.getUUID()) != -1) {
res.status(200).send("Instructor"); // send something else??
Copy link
Collaborator

Choose a reason for hiding this comment

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

(nitpick) Delete the comments

Copy link
Collaborator

@daniel-d-truong daniel-d-truong left a comment

Choose a reason for hiding this comment

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

Solid work! I am having issues running the routes though so let's sync up sometime today before merging and see why that's happening (I assume it worked on your end?)

Copy link
Collaborator

@daniel-d-truong daniel-d-truong left a comment

Choose a reason for hiding this comment

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

lgtm! just do last quick change on what the route sends and then feel free to merge

Copy link
Collaborator

@daniel-d-truong daniel-d-truong left a comment

Choose a reason for hiding this comment

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

lgtm

@pramithready pramithready merged commit 7d97f7b into master May 20, 2020
@daniel-d-truong daniel-d-truong linked an issue May 20, 2020 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Finish rest of User Routes
2 participants