cofacts

#200 Listing replies after selecting viewed article

#202 Convert GraphQL userArticleLinks to cursor-based pagination

Comment on #202 Convert GraphQL userArticleLinks to cursor-based pagination

Rebase and merge 看起來不會有 merge commit 呢囧 我還是用最傳統的 merge 好了，比較能從 commit 追 PR

#161 LIFF page for viewed article list

#203 Viewed message pagination mechanism

Comment on #201 Feature/163 insert data

<https://github.com/godgunman|@godgunman> 你484改完忘記push

#206 Add google analytics to LIFF

• Google analytics setup for LIFF • sends pageview on page change • sends user timing on LIFF load • record redirect count as redirect event

Comment on #206 Add google analytics to LIFF

*Pull Request Test Coverage Report for <https://coveralls.io/builds/31779780|Build 978>* • *0* of *0* changed or added relevant lines in *0* files are covered. • No unchanged relevant lines lost coverage. • Overall coverage remained the same at *98.379%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*

Comment on #270 Hotfix/minor bugs

user id is used to identify whether the current user added the category as well (so that they can remove those categories added by themselves)(in line 298), `disableVote` may be a bit confusing. it's good to extract the author check logic to upper component though

Comment on #270 Hotfix/minor bugs

as for the mutation part it's certainly difficult to put them in appropriate place, `useMutation` is a hook and it uses closure to get required params, this means that we should put the params and `useMutation` in the same function, and at the same time consist the calling order so that it won't break hook rules

Comment on #269 Feature/reply editor toobar

1. `replyReference` does not look like a variable that is really needed. If we need this variable, please use `const` and replace `reply.text + '\n'` with `replyReference` instead. 2. Please remove unused comment. 3. Seems that we did not copy the reply reference to `ReferenceInput` yet?

Review on #269 Feature/reply editor toobar

The unit test looks clearer now :) Thanks! I think we are good to go after we implement reference copying on "Add this reply to my reply" button.

Review on #270 Hotfix/minor bugs

LGTM! I am seeing this warning when running `npm run dev`, not sure if it will cause errors: ``` Warning: fragment with name ArticleWithCategories already exists. graphql-tag enforces all fragment names across your application to be unique; read more about this in the docs: <http://dev.apollodata.com/core/fragments.html#unique-names> ``` Let's see it on staging.

#270 Hotfix/minor bugs

#271 [Trivial] Fix trendline

*Before* <https://user-images.githubusercontent.com/108608/86210185-babaf000-bba6-11ea-9111-6c086ffce5b0.png|image> *After* <https://user-images.githubusercontent.com/108608/86210121-a37c0280-bba6-11ea-9a82-471e8e69d367.png|image>

Review on #271 [Trivial] Fix trendline

LGTM

Review on #271 [Trivial] Fix trendline

#271 [Trivial] Fix trendline

Comment on #269 Feature/reply editor toobar

Thanks for adding `reference`. I think we should use `reply.reference` directly instead of composing from `reply.hyperlinks`. Sometimes the author of the reply may <https://old.cofacts.org/reply/ORLVmHEBrhVJn3LNJnmn|put some info in reply's reference field>, such as adding section title between hyperlinks, or provide description to URLs that is more concise to hyperlink titles.

#164 User settings LIFF

Review on #269 Feature/reply editor toobar

LGTM! Thanks for the fix, let's see it in staging :sunglasses:

#269 Feature/reply editor toobar

#207 Feature/165 cron notify

<https://github.com/cofacts/rumors-line-bot/issues/165|#165>

Comment on #207 Feature/165 cron notify

*Pull Request Test Coverage Report for <https://coveralls.io/builds/31796635|Build 984>* • *26* of *44* *(59.09%)* changed or added relevant lines in *4* files are covered. • No unchanged relevant lines lost coverage. • Overall coverage decreased (*-4.7%*) to *93.679%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*

image.png

facebook.com

Richard Lee

LIFF SDK 現在也可以用 NPM 安裝了。另外安裝 NPM SDK，開發環境還有型別跟自動補完支援，寫起來效率滿分！

Comment on #207 Feature/165 cron notify

If we put cron job and web server together, there may be a case that if we have multiple instances for web server, the cron job will be triggered once for each instance, causing the job be run multiple times. I would suggest we just prepare a standalone NodeJS script that can be invoked via command line. On other environments, people can use native cron tab to trigger the script; and on Heroku, we can use <https://devcenter.heroku.com/articles/scheduler|Heroku scheduler> to schedule a run.

Comment on #207 Feature/165 cron notify

Since we already use `date-fns` in our application, I think we should use <https://date-fns.org/v2.14.0/docs/add|`date-fns/add`> directly, instead of implementing our own function.

Review on #207 Feature/165 cron notify

I haven't reviewed the core logic in determine who to notify. Just to provide some thought on how cron job can be defined and how user can be notified. I love the test cases, they helped me a lot when understanding how each utility function works :nerd_face:

Comment on #207 Feature/165 cron notify

I wonder if `push` is used anywhere yet?

Comment on #207 Feature/165 cron notify

I would suggest we send flex message with button when doing multicast, so that we can hide complex LIFF URLs from users using a button with URI action.

Comment on #207 Feature/165 cron notify

If there are multiple instances running the web server, this may cause the cron job being run once for each instance / process. I would suggest we just implement a NodeJS script that can be run using command line. On normal production environment, the devOp can use ordinary cron tab to trigger the job; on our production environment on Heroku, we can use <https://devcenter.heroku.com/articles/scheduler|Heroku scheduler>. In this case, we won't need `CronJob` library; we just need to make sure the script works when run on CLI, just like the <https://github.com/cofacts/rumors-api/blob/master/src/scripts/cleanupUrls.js#L136-L138|hyperlink cleaning job> in rumors-api.

#207 Feature/165 cron notify

Comment on #207 Feature/165 cron notify

no, currently we just use `multicast` and `notify`.

#272 hotfix: add category bug

Add user id field to CreateArticleCategory

g0v.hackmd.io

rumors-line-bot 過去傳過訊息 implementation - HackMD

Comment on #201 Feature/163 insert data

Why don't we just use mongo client for a find query? (Also for L118) The main point of my previous comment was because `UserSetting.findOrInsertByUserId` will mutate the database to make the test pass, even when the function under test (`singleUserHandler`) does not work as expected. It did not mean we should avoid using `mongoClient` entirely ._.

Review on #201 Feature/163 insert data

Thanks for updating the PR accordingly! Named a few suggestions and added conclusion from the discussion <https://g0v-tw.slack.com/archives/C2PPMRQGP/p1593850412237300|on Slack> today.

Comment on #201 Feature/163 insert data

There is already a static method called `find` in `UserArticleLink`, which is used by GraphQL resolver for `Query.userArticleLinks`. `find` was added before this branch previously branched out and is now in the file after your rebase. I think we should only leave one endpoint that lists user-article links of a user. I like the naming `findByUserId` because it is more clear (and considering that we need also to find by article id in <https://github.com/cofacts/rumors-line-bot/pull/207|#207>). But the pagination mechanism is also required by the GraphQL API. Would you merge the implementation of the two find methods into `findByUserId()`, remove `find()` and update the resolver (`graphql/resolvers/Query.js`) accordingly?

Comment on #201 Feature/163 insert data

Since `choosingReply` does not involve `UserArticleLink`, I think we should not include these in test file, or it may confuse readers thinking that `choosingReply` has used `UserArticleLink`.

Comment on #201 Feature/163 insert data

As discussed in slack today, we should use an API that also writes `lastViewedAt`. Also, please add `lastViewedAt` as a required field in `userArticleLink.json` to ensure that `lastViewedAt` always exists in DB.

Comment on #201 Feature/163 insert data

Actually there is another method called `findOrInsertByUserIdAndArticleId` in `UserArticleLink` model, doing almost the same thing as `updateTimestamps` does, but it's not used anywhere. I suggest we should leave only one of such method in `UserArticleLink`.

Comment on #207 Feature/165 cron notify

1. Accepting inputting multiple `articleIds` and search by `{articleId: {$in: articleIds}}` can provide better flexibility. It should work more efficiently as well. 2. Since this API still returns `UserArticleLink`s instead of users, the `UserList` in its naming is a bit confusing. suggest using something like `findByArticleIds`.

Comment on #207 Feature/165 cron notify

According to <https://g0v-tw.slack.com/archives/C2PPMRQGP/p1593850412237300|our discussion on slack>, all `userArticleLink` should have `lastViewedAt`. We may need to update these fixtures to match that.

Comment on #207 Feature/165 cron notify

I suggest we move some of the `articleReply`'s `createdAt` outside of the queried time range because • `ListArticle(repliedAt)` includes an article as long as it has _one_ `articleReply` within the time range, thus it is possible for its results having _some_ article-replies outside the time range • Including such possibility in the test case can better cover all scenarios • Currently there are already plenty of `articleReplies` in the fixture, but all of them in the same time range. Moving some of them outside the time range can increase diversity of fixture combinations.

Review on #207 Feature/165 cron notify

I have viewed the logic of cron jobs. Thanks for the contribution! As our <https://datastudio.google.com/u/0/reporting/18J8jZYumsoaCPBk9bdRd97GKvi_W5v-r/page/ckUQ|open data analytics> shows, sometimes there will be more than 100 articles replied in a day. I have made some suggestions to enhance robustness of cron jobs by processing in batches. Also there are some suggestions about diversity of test fixtures.

Comment on #207 Feature/165 cron notify

The combination of `Promise.all` and `....map()` will cause `UserSettings.findOrInsertByUserId` being invoked in parallel. If there are `N` users to notify, this will trigger `N` queries to database _at once_. This can be dangerous when a very popular article (&gt;100 user article links) receives a new reply. I would suggest • Divide `Object.keys(notificationList)` (user ids) into batches, size of each batch controlled by a constant. • Use for-loop with `await` in loop to make sure only 1 batch is processed at a time. • Create a new static method on `UserSettings` that reads user settings in batch and does not perform upsert. • use `findAll({userId: {$in: userIds}})` in the new method to receive batch of user settings • there is no point inserting `UserSettings` in cron jobs • skip a user when the user does not have `UserSettings` (it should never happen though, just in case)

Comment on #207 Feature/165 cron notify

Sending one notification for each user that receive notification may <https://developers.google.com/analytics/devguides/collection/analyticsjs/limits-quotas|hit this quota> if there are more than 20 users to send notification: &gt; Each gtag.js and analytics.js tracker object starts with 20 hits that are replenished at a rate of 2 hits per second I think for now we can just send one event to GA that records the number of notifications we sent this time (in <https://developers.google.com/analytics/devguides/collection/analyticsjs/events|event value> so that the number adds up when query for multiple days).

Comment on #207 Feature/165 cron notify

These comments will be sent as part of GraphQL. I think we can remove these comments because the variable name already implies the same meaning.

Comment on #207 Feature/165 cron notify

Suggest adding `status:NORMAL` to exclude deleted replies

Comment on #207 Feature/165 cron notify

For processing the articles, I would suggest 1. Use pagination to get _all_ articles that match the filter, not just first `N` articles. 2. Sequentially process each batch of `N` articles. Design APIs so that it can handle multiple articles at once. ``` // Async generator that gets a batch of articles with articleReply between `from` and `to`. // The generator encapsulates complex pagination logic so that the function using it can focus on // batch processing logic without worrying pagination. // async function* getArticlesInBatch(from, to) { // Get pageInfo outside the loop since it's expensive for rumors-api const { data: { ListArticles: { pageInfo: {lastCursor} } } } = await gql`...`({from, to}); let after = undefined; while(lastCursor !== after) { // Actually loads `edges` and process. const { data: { ListArticles } } = await gql`...`({from, to, after}); yield ListArticles.edges.map(({node}) =&gt; node); // next gql call should go after the last cursor of this page after = ListArticles.edges[ListArticles.edges.length - 1].cursor; } } async function getNotificationList(lastScannedAt, nowWithOffset) { const result = {}; // for loop ensures that only one batch will be processed at a time, so that we do not // make a bunch of queries to our MongoDB at once. // for await (const articles of getArticlesInBatch(lastScannedAt, nowWithOffset)) { // Process the batch // Make an API that processes a batch of articleIds, instead of only one ID at a time const userArticleLinks = await UserArticleLink.findByArticleIds(articles.map(({id}) =&gt; id)); userArticleLinks.forEach(/* Logic that populates result */) } return result; } ```

Comment on #201 Feature/163 insert data

Should we test if `unfollow` event changes `allowNewReplyUpdate` to false?

Comment on #272 hotfix: add category bug

Still can't remove a category added by myself from the article detail page. The category first seems to deleted, but re-appear after refresh. I am seeing this warning in console. Not sure if it is relevant. &gt; Warning: fragment with name ArticleWithCategories already exists. &gt; graphql-tag enforces all fragment names across your application to be unique; read more about &gt; this in the docs: <http://dev.apollodata.com/core/fragments.html#unique-names|http://dev.apollodata.com/core/fragments.html#unique-names> I found that • there is no `status` exist in fetched `articleCategories` in apollo cache. • when querying article's `articleCategories` we did not specify `(status: NORMAL)` in GraphQL. I guess that's probably why all `articleCategories` are fetched and displayed. Since the display logic actually do not involve `status`, I suggest specifying `(status: NORMAL)` when querying `articleCategories` in article detail page.

Comment on #201 Feature/163 insert data

Hmm, because you say `using MongoDb commands` :joy::joy: . It's a bit confusing for me, but doing mongo CLI is also quick, then I made it haha

Comment on #201 Feature/163 insert data

How about we move `find()` into `src/database/models/base.js` ? So that all models can use `find()` just like they already have `findOneAndUpdate`. As a result, `findByUserId()` is for listing user-article links in UserArticleLink, and `find()` is for general use in all models.

Comment on #201 Feature/163 insert data

But mongodb already have a method named `find()`. I would like to see `findXXX` methods directly invoking methods of mongodb client. Adding one more layer to `find()` is a bit over-engineered to me.

Comment on #201 Feature/163 insert data

Agree adding `find()` to `Base` so that `Base` API is more consistent :woman-gesturing-ok:

Review on #272 hotfix: add category bug

It's working properly now. Thanks for the fix!

#272 hotfix: add category bug

#179 ClaimReview search API

Discussion: <https://g0v.hackmd.io/@mrorz/cofacts-meeting-notes/%2F%40mrorz%2FByKAmKITU|https://g0v.hackmd.io/@mrorz/cofacts-meeting-notes/%2F%40mrorz%2FByKAmKITU> &gt; orz: Google 有針對 ClaimReview 做一個專門的 search box 耶：<https://toolbox.google.com/factcheck/explorer|https://toolbox.google.com/factcheck/explorer> （預設只會顯示瀏覽器語言的 fact-check result） &gt; 好像也有 Search API: <https://toolbox.google.com/factcheck/apis|https://toolbox.google.com/factcheck/apis> &gt; Lucien: 要不要做在 API server &gt; 就能查完直接送進 DB &gt; 或者轉換為一種 reply

#273 Revamp deleted reply list in article page

Spec: <https://g0v.hackmd.io/dYT7zCPGQiKsTne7-kkybw|https://g0v.hackmd.io/dYT7zCPGQiKsTne7-kkybw> Directions: only show deleted article-reply to its author