-
Notifications
You must be signed in to change notification settings - Fork 7
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
[ft-165412937] add reading stats #68
Conversation
91ad001
to
ad749b2
Compare
src/controllers/ArticleController.js
Outdated
const { tagList } = req.body; | ||
const newArticle = await Article.create({ | ||
userId: req.user.id || 0, | ||
slug: helpers.generator.slug(req.body.title), | ||
title: req.body.title.trim(), | ||
description: req.body.description.trim(), | ||
body: req.body.body.trim(), | ||
coverUrl: image.image.original, | ||
// coverUrl: image.image.original, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already removed
src/controllers/ArticleController.js
Outdated
const { tagList } = req.body; | ||
const newArticle = await Article.create({ | ||
userId: req.user.id || 0, | ||
slug: helpers.generator.slug(req.body.title), | ||
title: req.body.title.trim(), | ||
description: req.body.description.trim(), | ||
body: req.body.body.trim(), | ||
coverUrl: image.image.original, | ||
// coverUrl: image.image.original, | ||
coverUrl: req.body.coverUrl || 'placeholder.png', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove placeholder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This too was already removed
src/controllers/UserController.js
Outdated
@@ -1,5 +1,5 @@ | |||
import dotenv from 'dotenv'; | |||
import { User } from '../queries'; | |||
import { User, findAndCountAll, create } from '../queries'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does findAndCountAll
query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findAndCountAll return both data and total count.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
count for what? it should be named appropriately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have renamed to an appropriate name.
e1a232e
to
a087945
Compare
src/controllers/UserController.js
Outdated
@@ -1,5 +1,5 @@ | |||
import dotenv from 'dotenv'; | |||
import { User } from '../queries'; | |||
import { User, findAndCountAll, create } from '../queries'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
count for what? it should be named appropriately
a087945
to
4711a44
Compare
src/controllers/UserController.js
Outdated
@@ -1,5 +1,5 @@ | |||
import dotenv from 'dotenv'; | |||
import { User } from '../queries'; | |||
import { User, getAll, create } from '../queries'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be something like getAllRatings
, use the object approach. Have an object ReadingStat and inside that have all the methods you want. That way you can name them the way you have it above. getAll
and Create
. This makes it easier to read and use. You will import the ReadingStat
object and anywhere you want to use it you will invoke ReadingStat.create
or ReadingStat.getAll
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
0116e0f
to
83e81b8
Compare
83e81b8
to
e98563e
Compare
What does this PR do?
Description of Task to be completed?
Have the following endpoints working
How should this be manually tested?
Any background context you want to provide?
What are the relevant pivotal tracker stories?
Screenshots (if appropriate)
Questions: