Fixes #3615: Add author, title, publishDate fields to '/' route #3764

liutng · 2022-11-17T01:22:50Z

Issue This PR Addresses

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

In this PR, an extra query param expand is added to the URL /posts in posts services.
When expand=1, there will be extra data of author, title, and publishDate returned.
When expand does not equal 1, or expand does not present in the request, the request will return the data like before.

Steps to test the PR

Load the posts page in the react-native app, and observe if the extra fields of data are returned by /posts. This requires the react-native app to add "expand=1" to its request query params of /posts as well.

Here's what does the request and data look like.

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 · 2022-11-17T01:22:56Z

P-DR0ZD

Looks Good

liutng · 2022-11-17T01:50:05Z

Looks Good

Thanks.

humphd · 2022-11-17T02:09:05Z

src/api/posts/test/posts.test.js

@@ -69,6 +69,19 @@ describe('/posts', () => {
    expect(res.body.length).toBeGreaterThan(0);
  });

+  test('request posts with a valid expand query param', async () => {


Please add a test for expand=1 as well.

I tried to cover this scenario in my test but it either returns empty data or timeout, although the API works fine if I were requesting with my browser or Postman. Got any clue what might cause this problem?

Debug it, add logging, etc. You should have 150 posts created before this test runs. We can't land this change without a test that proves it works.

humphd · 2022-11-17T02:10:00Z

src/api/posts/src/routes/posts.js

-    ids
+  let data;
+  if (expand === 1) {
+    data = await Promise.all(


This can fail, needs try/catch

Thank for the suggestion, a try/catch block is now added to wrap Promise.all().

When you say "is now added," there is nothing here. Please push your changes.

Sorry, I forgot to push my code, it's now pushed.

humphd

Looks like the test changes are still pending? What's happening with those?

liutng · 2022-11-17T23:41:21Z

Looks like the test changes are still pending? What's happening with those?

I'm still working on the test where expand=1.

liutng · 2022-11-18T00:40:50Z

Sorry for the delay. Finally, the test is working. I added extra feed data to posts.test.js to make it work. The code has been pushed now.

humphd · 2022-11-18T18:50:30Z

src/api/posts/test/posts.test.js

+    const res = await request(app).get('/?page=1&expand=1');
+    expect(res.status).toEqual(200);
+    // This will depend on the env value, so as long as we get back something.
+    expect(res.body.length).toBeGreaterThan(0);


This isn't enough. In the case that you include expand=1, we'll expect the shape of the objects returned in the array to be different. Please add some logic to the tests to deal with the case of expand=1 and without, looking at the properties in the objects.

Got it, I've pushed a revised test that now checks the extra properties in the return data when expand=1.

This is getting better, but still doesn't test what we need.

Use Jest's arrayContaining and objectContaining to look for one item that has the shape/values you expect in the results returned.

src/api/posts/src/routes/posts.js

humphd · 2022-11-20T15:08:04Z

src/api/posts/src/routes/posts.js

@@ -60,14 +60,31 @@ posts.get('/', validatePostsQuery(), async (req, res, next) => {
    first: `/posts?per_page=${perPage}&page=${1}`,
    last: `/posts?per_page=${perPage}&page=${Math.floor(postsCount / perPage)}`,
  });
-  res.json(
-    ids
+  let data = {};


nit: don't bother assigning a value here, since you'll throw it away in the if/else that follows. Another pattern you could use is to move the if/else to a function:

try { const data = await prepareData(ids, expand === 1); res.json(data); } ...

And your function could look like this (untested):

const prepareData = async (ids, shouldExpand) { if (shouldExpand) { return Promise.all( ids.map(async (id) => { // obtain the corresponding feed to populate the author's name to the return data. const post = await Post.byId(id); return { id, url: `${postsUrl}/${id}`, author: post.feed.author, title: post.title, publishDate: post.published, }; }) ); } else return ids .map((id) => ({ id, url: `${postsUrl}/${id}`, })) ); };

humphd · 2022-11-20T15:08:35Z

src/api/posts/test/posts.test.js

+      link: 'http://foo.link',
+      etag: 'etag',
+      lastModified: new Date('2009-09-07T22:23:00.544Z'),
+      githubUsername: 'githubusr',


s/githubusr/githubUsername/ to make it even more clear.

Sorry, what does this mean? Should I change the field name of githubUsername to something else? Or the value githubusr?

The value. It's easiest if the property name and value match (e.g., like etag above).

Got it, thank you for the suggestion, it's already fixed now and pushed.

humphd · 2022-11-20T15:10:55Z

src/api/posts/test/posts.test.js

+    const res = await request(app).get('/?page=1&expand=1');
+    expect(res.status).toEqual(200);
+    // This will depend on the env value, so as long as we get back something.
+    expect(res.body.length).toBeGreaterThan(0);


This is getting better, but still doesn't test what we need.

Use Jest's arrayContaining and objectContaining to look for one item that has the shape/values you expect in the results returned.

humphd

Nice, almost there.

humphd · 2022-11-21T00:14:03Z

src/api/posts/src/routes/posts.js

@@ -7,11 +7,35 @@ const postsUrl = process.env.POSTS_URL || '/';

 const posts = Router();

+const prepareData = async (ids, shouldExpand = false) => {
+  if (shouldExpand) {
+    const data = await Promise.all(


No need for data or await here, just return the Promise directly:

return Promise.all(

Yes, I tried directly to return Promise.all() without prefixing await. It will cause lint to complain. Here is what the complain looks like

OK, eslint is wrong here, but that's OK. Change to this:

const prepareData = (ids, shouldExpand = false) => { if(shouldExpand) { return Promise.all(...) } return Promise.resolve(...) }

Hi, I pushed a new commit but eslint still complains.

humphd · 2022-11-21T15:17:32Z

src/api/posts/src/routes/posts.js

@@ -9,7 +9,7 @@ const posts = Router();

 const prepareData = async (ids, shouldExpand = false) => {


Get rid of async here, we aren't going to await anymore.

Got it. A new commit is pushed.

humphd

This looks good to me, nice work. Once this goes green on CI, please squash all 12 commits into 1, and rebase on latest master.

liutng · 2022-11-21T15:26:39Z

Ok, I will do it now. Thank you for reviewing my code.

Co-authored-by: AmasiaNalbandian <[email protected]>

… file (Seneca-CDOT#3722) * Fixes Seneca-CDOT#3692 by adding database info to docs

* Delete text "snippet" * Change solution approach to add eclipses * Fix Twitch typo * Fix class names

Update dependency row bottom border color to be lighter in dark mode Make border style consistent across all rows Make border style important Co-authored-by: Kevan Yang <[email protected]> Remove extra whitespace Make dependency info cell border style important

…le (Seneca-CDOT#3752) Co-authored-by: AmasiaNalbandian <[email protected]>

…DOT#3751)

* Fixed lint errors * Wrapped Promise.all() with try/catch block because it might fail. * Added test for `/post/` when expand=1 * Increased test granularity on the test * Increased the test strength of expand=1 * Reduced code redundancy in posts.js * Changed the pseudo-data in `posts.test.js` to make it more rational. * Removed redundant variable assignment. * Removed async in parepareData()

liutng · 2022-11-21T16:49:14Z

I tried everything I could to fix this PR. Maybe this Pull Request is beyond repairable.. Should I close this one and open a new one?

humphd · 2022-11-21T17:00:20Z

Sure, that's a good idea. It would be useful to figure out what you did wrong, so you can fix in the future. This workflow is something you need to master. Let me know if you need help.

liutng · 2022-11-21T17:41:43Z

Thank you, I will not hesitate to contact you when I need help and also the lesson I've learned from this time is never use any git command if I'm not aware of what this command does.

Fixed issue Seneca-CDOT#3615: added more values to /post

aa3046e

vercel bot deployed to Preview November 17, 2022 01:24 View deployment

vercel bot deployed to Preview November 17, 2022 01:39 View deployment

P-DR0ZD previously approved these changes Nov 17, 2022

View reviewed changes

manekenpix changed the title ~~Fix: Issue #3615~~ Fixes #3615: Add author, title, publishDate fields to '/' route Nov 17, 2022

humphd requested changes Nov 17, 2022

View reviewed changes

manekenpix assigned liutng Nov 17, 2022

liutng dismissed P-DR0ZD’s stale review via 8055e5e November 17, 2022 16:14

vercel bot deployed to Preview November 17, 2022 16:15 View deployment

humphd reviewed Nov 17, 2022

View reviewed changes

vercel bot deployed to Preview November 18, 2022 00:42 View deployment

TueeNguyen self-requested a review November 18, 2022 18:40

humphd requested changes Nov 18, 2022

View reviewed changes

vercel bot deployed to Preview November 18, 2022 19:35 View deployment

Kevan-Y reviewed Nov 19, 2022

View reviewed changes

src/api/posts/src/routes/posts.js Outdated Show resolved Hide resolved

humphd requested changes Nov 20, 2022

View reviewed changes

vercel bot deployed to Preview November 20, 2022 16:58 View deployment

vercel bot deployed to Preview November 20, 2022 18:57 View deployment

humphd requested changes Nov 21, 2022

View reviewed changes

vercel bot deployed to Preview November 21, 2022 15:15 View deployment

humphd requested changes Nov 21, 2022

View reviewed changes

vercel bot deployed to Preview November 21, 2022 15:23 View deployment

humphd previously approved these changes Nov 21, 2022

View reviewed changes

liutng dismissed humphd’s stale review via 52e8ac5 November 21, 2022 15:31

vercel bot deployed to Preview November 21, 2022 15:33 View deployment

cindyorangis and others added 22 commits November 21, 2022 11:31

update engine to node.js 16 and deps for satellite service

879134a

update engine to node.js 16 and deps for image service

5a7b9bb

bump satellite version to 1.29.0

07db27f

chore: update moment.js (Seneca-CDOT#3733)

6b4e806

update satellite to v.1.29.0 in all telescope services

5b06d61

upgraded post service node version from 14 to 16 (Seneca-CDOT#3740)

336459e

Co-authored-by: AmasiaNalbandian <[email protected]>

upgraded node in feed discovery from v12 to 16 (Seneca-CDOT#3741)

64bc23e

Co-authored-by: AmasiaNalbandian <[email protected]>

Fixes Seneca-CDOT#3692 by adding database info to environment-setup…

70b60e0

… file (Seneca-CDOT#3722) * Fixes Seneca-CDOT#3692 by adding database info to docs

update engine to node.js 16 and deps for search service

90f1ccf

Make RSS on Sign-up review page into snippets with hover display

15207a6

* Delete text "snippet" * Change solution approach to add eclipses * Fix Twitch typo * Fix class names

update engine to node.js 16 and deps for sso service (Seneca-CDOT#3748)

5b6b079

changed link color to be blue if not visited on blogs instead of purp…

e74cf9e

…le (Seneca-CDOT#3752) Co-authored-by: AmasiaNalbandian <[email protected]>

Bulgarian translation of overview page

25a729f

fixed bulgarian overview page location

3f9f32c

I had a bulgarian speaker to read, correct grammer

d21f7f3

Improve google Lighthouse accessibly score (Seneca-CDOT#3735)

2e27226

Release 3.1.1

32e496b

Fixes Seneca-CDOT#3687: Auto select feed if only one exists (Seneca-C…

75379ec

…DOT#3751)

change font of list items (Seneca-CDOT#3754)

fe0832a

Removed unused variables and wrote more tests

e6658e8

liutng force-pushed the issue-3165-fix branch from e72214c to 7ab7846 Compare November 21, 2022 16:46

vercel bot deployed to Preview November 21, 2022 16:49 View deployment

liutng closed this Nov 21, 2022

liutng deleted the issue-3165-fix branch November 21, 2022 17:43

liutng mentioned this pull request Nov 21, 2022

Fixed issue #3615(Reopen) #3771

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #3615: Add author, title, publishDate fields to '/' route #3764

Fixes #3615: Add author, title, publishDate fields to '/' route #3764

liutng commented Nov 17, 2022 •

edited by manekenpix

Loading

gitpod-io bot commented Nov 17, 2022

P-DR0ZD left a comment

liutng commented Nov 17, 2022

humphd Nov 17, 2022

liutng Nov 17, 2022

humphd Nov 17, 2022

humphd Nov 17, 2022

liutng Nov 17, 2022

humphd Nov 17, 2022

liutng Nov 17, 2022

humphd left a comment

liutng commented Nov 17, 2022

liutng commented Nov 18, 2022 •

edited

Loading

humphd Nov 18, 2022

liutng Nov 18, 2022

humphd Nov 20, 2022

humphd Nov 20, 2022

humphd Nov 20, 2022

liutng Nov 20, 2022

humphd Nov 20, 2022

liutng Nov 20, 2022

humphd Nov 20, 2022

humphd left a comment

humphd Nov 21, 2022

liutng Nov 21, 2022

humphd Nov 21, 2022

liutng Nov 21, 2022 •

edited

Loading

humphd Nov 21, 2022

liutng Nov 21, 2022

humphd left a comment

liutng commented Nov 21, 2022 •

edited

Loading

liutng commented Nov 21, 2022

humphd commented Nov 21, 2022

liutng commented Nov 21, 2022

		@@ -9,7 +9,7 @@ const posts = Router();

		const prepareData = async (ids, shouldExpand = false) => {

Fixes #3615: Add author, title, publishDate fields to '/' route #3764

Fixes #3615: Add author, title, publishDate fields to '/' route #3764

Conversation

liutng commented Nov 17, 2022 • edited by manekenpix Loading

Issue This PR Addresses

Type of Change

Description

Steps to test the PR

Checklist

gitpod-io bot commented Nov 17, 2022

P-DR0ZD left a comment

Choose a reason for hiding this comment

liutng commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

liutng commented Nov 17, 2022

liutng commented Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liutng Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

liutng commented Nov 21, 2022 • edited Loading

liutng commented Nov 21, 2022

humphd commented Nov 21, 2022

liutng commented Nov 21, 2022

liutng commented Nov 17, 2022 •

edited by manekenpix

Loading

liutng commented Nov 18, 2022 •

edited

Loading

liutng Nov 21, 2022 •

edited

Loading

liutng commented Nov 21, 2022 •

edited

Loading