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

Added jobs count to dashboard #2585

Merged
merged 1 commit into from Dec 15, 2021

Conversation

TueeNguyen
Copy link
Contributor

@TueeNguyen TueeNguyen commented Dec 8, 2021

Issue This PR Addresses

This fixes issue #2414 which is also a follow up of PR #2541

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

  • Added jobCountCard.hbs to display jobs in feeds queue in the backend
  • Added to src/api/status/src/js a new file queue-stats.js to fetch queue's information using posts API posts/feeds/info

To test this feature:

  • Added * or http://localhost/v1/posts/feeds/info to connectSrc in server.js
  • Change the fetch url in queue/index.js to http://localhost/v1/posts/feeds/info
  • Run pnpm run services:start posts in root folder and npm run dev in api/status, you'll get feeds/info initial response
  • (Optional) Run all services pnpm run services:start and pnpm start to start the backend feeds queue. Reload to see changes
2021-12-09.13-31-08.mp4

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Dec 8, 2021

Comment on lines 46 to 52
const resKeys = Object.keys(resBody.queueInfo);
for (let i = 0; i < resKeys.length; i += 1) {
if (allKeys.indexOf(resKeys[i]) < 0 || typeof resBody.queueInfo[resKeys[i]] !== 'number') {
bool = false;
break;
}
}
Copy link
Member

@manekenpix manekenpix Dec 9, 2021

Choose a reason for hiding this comment

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

Suggested change
const resKeys = Object.keys(resBody.queueInfo);
for (let i = 0; i < resKeys.length; i += 1) {
if (allKeys.indexOf(resKeys[i]) < 0 || typeof resBody.queueInfo[resKeys[i]] !== 'number') {
bool = false;
break;
}
}
Object.keys(resBody.queueInfo).forEach((key) =>
if (!allKeys.includes(key) || typeof key !== 'number')
return false;

(didn't test it, but it's probably something very close to this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used every instead of forEach because forEach doesn't support break or return

@humphd
Copy link
Contributor

humphd commented Dec 9, 2021

This needs a rebase to update for changes to posts and status.

Copy link
Contributor

@Kevan-Y Kevan-Y left a comment

Choose a reason for hiding this comment

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

LGTM, nice feature

Copy link
Contributor

@Andrewnt219 Andrewnt219 left a comment

Choose a reason for hiding this comment

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

The dashboard LGTM. Not quite sure how to review the backend.

@TueeNguyen
Copy link
Contributor Author

The dashboard LGTM. Not quite sure how to review the backend.

The backend is ok except for some test refactoring, I’ve got the backend approved in another PR

@humphd
Copy link
Contributor

humphd commented Dec 10, 2021

Where is this at? It says "Draft"

src/api/status/src/js/queue-stats.js Outdated Show resolved Hide resolved
@TueeNguyen TueeNguyen self-assigned this Dec 12, 2021
@humphd
Copy link
Contributor

humphd commented Dec 14, 2021

This needs to be rebased to pick up the changes in affected files, and I'd like to see it get finished asap so we can land it this week. Can I get an update please @TueNguyen2911?

@TueeNguyen
Copy link
Contributor Author

Of course, I'll rebase this this afternoon after my last final! Sorry for the delay

@TueeNguyen
Copy link
Contributor Author

@humphd, I've rebased it, can you take a look?

Andrewnt219
Andrewnt219 previously approved these changes Dec 15, 2021
Copy link
Contributor

@Andrewnt219 Andrewnt219 left a comment

Choose a reason for hiding this comment

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

LGTM 👍.


module.exports = async function getJobCount() {
try {
const data = await fetch(`${process.env.POST_URL}/feeds/info`, { method: 'GET' });
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const data = await fetch(`${process.env.POST_URL}/feeds/info`, { method: 'GET' });
const data = await fetch(`${process.env.POSTS_URL}/feeds/info`, { method: 'GET' });

Kevan-Y
Kevan-Y previously approved these changes Dec 15, 2021
Copy link
Contributor

@Kevan-Y Kevan-Y left a comment

Choose a reason for hiding this comment

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

LGMT, great job 👍

src/api/posts/test/feeds.test.js Outdated Show resolved Hide resolved
src/api/status/src/js/queue-stats.js Outdated Show resolved Hide resolved
Comment on lines 79 to 81
return Object.keys(resBody.queueInfo).every((key) => {
if (!allKeys.includes(key) || typeof resBody.queueInfo[key] !== 'number') {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return Object.keys(resBody.queueInfo).every((key) => {
if (!allKeys.includes(key) || typeof resBody.queueInfo[key] !== 'number') {
return false;
return Object.keys(resBody.queueInfo).every((key) => allKeys.includes(key) && typeof resBody.queueInfo[key] === 'number')

This can be shorted. Affirmative is usually easier to read too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I missed this part in the previous review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, thank you for the suggestion

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

R+ with CI passing.

@Andrewnt219 Andrewnt219 merged commit 3c9dd6d into Seneca-CDOT:master Dec 15, 2021
@TueeNguyen
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants