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

#167190450 Implement pagination for Articles #22

Merged
merged 1 commit into from
Aug 6, 2019

Conversation

chiboycalix
Copy link
Contributor

@chiboycalix chiboycalix commented Aug 2, 2019

What does this PR do?

  • This PR Set's up pagination support for Articles

Description of Task to be completed?

  • Write a controller function to handle pagination support

How should this be manually tested?

  • run git clone [repo name] to clone the repo
  • run npm install to install all necessarry dependencies
  • run npm run dev to start up the development server
  • go to POSTMAN type https://5040/articles/?limit=articlenumber&page=pagenumber. This is a GET request

Any background context you want to provide?

  • you can decide to specify only a page number or only a limit number and the default values for the page number or limit number would be used respectively

What are the relevant pivotal tracker stories?

#167190450

Screenshots (if appropriate)

Screenshot 2019-08-02 at 6 44 58 PM

Questions:

  • none

@chuxmykel chuxmykel temporarily deployed to a-haven-staging-pr-22 August 2, 2019 17:57 Inactive
@chiboycalix chiboycalix force-pushed the ft/articles-pagination-167190450 branch from 15af4ac to 26d70eb Compare August 3, 2019 06:36
@chuxmykel chuxmykel temporarily deployed to a-haven-staging-pr-22 August 3, 2019 06:36 Inactive
@chiboycalix chiboycalix force-pushed the ft/articles-pagination-167190450 branch from 26d70eb to fab9f9e Compare August 3, 2019 07:47
@chuxmykel chuxmykel temporarily deployed to a-haven-staging-pr-22 August 3, 2019 07:47 Inactive
@chiboycalix chiboycalix force-pushed the ft/articles-pagination-167190450 branch from fab9f9e to e6e626d Compare August 3, 2019 10:58
@chuxmykel chuxmykel temporarily deployed to a-haven-staging-pr-22 August 3, 2019 10:58 Inactive
@chiboycalix chiboycalix force-pushed the ft/articles-pagination-167190450 branch from e6e626d to 953329e Compare August 3, 2019 11:18
@chuxmykel chuxmykel temporarily deployed to a-haven-staging-pr-22 August 3, 2019 11:19 Inactive
@chiboycalix chiboycalix force-pushed the ft/articles-pagination-167190450 branch from 953329e to e48a1b9 Compare August 3, 2019 11:55
@chuxmykel chuxmykel temporarily deployed to a-haven-staging-pr-22 August 3, 2019 11:55 Inactive
@chiboycalix chiboycalix force-pushed the ft/articles-pagination-167190450 branch from e48a1b9 to 03718a0 Compare August 3, 2019 11:57
@chiboycalix chiboycalix force-pushed the ft/articles-pagination-167190450 branch from 03718a0 to c95fe4f Compare August 5, 2019 08:25
Copy link
Contributor

@chuxmykel chuxmykel 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.

page = parseInt(page, 10) > 0 ? page : 1;
const offset = (page - 1) * limit;
const articles = await models.Article.findAndCountAll({
where: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious bro. Is there any reason why you specified an empty where constraint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for that question. The reason is that am not supposed to filter the returned article by any condition. If the story, for instance, required that I paginate the article and return articles that belong to a particular author, then I can include a condition in the where object. (e.g where: {authorId: req.user.id), then this will return only articles created by an author with the specified authorId.

Copy link
Contributor

@obayomi96 obayomi96 left a comment

Choose a reason for hiding this comment

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

LGTM

@chiboycalix chiboycalix added the Please review me Needs review label Aug 5, 2019
@chiboycalix chiboycalix force-pushed the ft/articles-pagination-167190450 branch 3 times, most recently from 4aa89c3 to de421fd Compare August 5, 2019 17:57
- setup pagination for articles
- write controller functions to handle pagination
- write unit test
[finishes #167190450]
@chiboycalix chiboycalix force-pushed the ft/articles-pagination-167190450 branch from de421fd to c2d8ab9 Compare August 5, 2019 17:58
Copy link
Contributor

@Hadeneekeh Hadeneekeh left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Lundii Lundii left a comment

Choose a reason for hiding this comment

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

Nice work. LGTM

Copy link
Contributor

@encodedBicoding encodedBicoding left a comment

Choose a reason for hiding this comment

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

LGTM

@topseySuave topseySuave merged commit 4125a6f into develop Aug 6, 2019
chiboycalix pushed a commit that referenced this pull request Aug 6, 2019
#167190450 Implement pagination for Articles
chiboycalix pushed a commit that referenced this pull request Aug 6, 2019
#167190450 Implement pagination for Articles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants