HackMD
在 LINE Chatbot 中串接 Google Analytics 的經驗分享 - HackMD
在 LINE Chatbot 中串接 Google Analytics 的經驗分享
small refactor, moving date range helper to util to be reused in analytics dataloader
depending on <https://github.com/cofacts/rumors-api/pull/188|#188> and <https://github.com/cofacts/rumors-api/pull/189|#189>
<https://coveralls.io/builds/32483673|Coverage Status> Coverage decreased (-0.02%) to 87.574% when pulling *<https://github.com/cofacts/rumors-api/commit/839bcd08f441b5d18d1b6e0015c6545c6a5e3b0f|839bcd0> on dateUtil* into *<https://github.com/cofacts/rumors-api/commit/7dc45064a84a9ae4e129d45dc77d1ff44270ae23|7dc4506> on fetchGA*.
Comment on #190 Analytics data loader
<https://coveralls.io/builds/32483677|Coverage Status> Coverage increased (+0.2%) to 87.78% when pulling *<https://github.com/cofacts/rumors-api/commit/095d1eaa1fad7697f9425e609889d5bd740c84fa|095d1ea> on analyticsDataLoader* into *<https://github.com/cofacts/rumors-api/commit/839bcd08f441b5d18d1b6e0015c6545c6a5e3b0f|839bcd0> on dateUtil*.
<https://coveralls.io/builds/32483773|Coverage Status> Coverage increased (+0.02%) to 87.798% when pulling *<https://github.com/cofacts/rumors-api/commit/969bc5b810aadad8c8722af076eee796e8093513|969bc5b> on articleStats* into *<https://github.com/cofacts/rumors-api/commit/095d1eaa1fad7697f9425e609889d5bd740c84fa|095d1ea> on analyticsDataLoader*.
Review on #188 script to fetch google analytics stats
Thanks for the commit! I have finished commenting on the script implementation. Going to inspect the test cases tonight.
Comment on #188 script to fetch google analytics stats
Since this try-catch statement covers the entire function, it is equivalent to doing try-catch in the call site of `upsertDocStats`. However, the latter allows the user of `upsertDocStats` to determine if they want to catch it. I would suggest removing try-catch here.
Comment on #188 script to fetch google analytics stats
I find it difficult to differentiate the use case between L291-294 and L307-315. A comment elaborating this case (edge-case handling across batches) would be helpful.
Comment on #188 script to fetch google analytics stats
dotenv variables should be string already, thus `''` can be omitted. For example, `process.env.PORT` is `5000` even without `''`.
Comment on #188 script to fetch google analytics stats
Seems that errors are all unexpected items that should be reported to Rollbar. It would be nice if we send them using `Rollbar.error(err)`.
Comment on #188 script to fetch google analytics stats
Some explanation on how to execute the script here (or in `README.md` instead) would be great. Newly introduced env vars in `.env.sample` needs some documentation in here (or in `README.md` instead).
Comment on #188 script to fetch google analytics stats
I suppose `row.metrics[0].values` are numbers in strings? If so, `parseInt(visits, 10)` or even `+visits` can convert strings to number. Use of eval is <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval#Never_use_eval!|highly discouraged>.
Comment on #188 script to fetch google analytics stats
Since it's <https://developers.google.com/analytics/devguides/reporting/core/v4/resource-based-quota|analytics 360 only function>, we will probably never uncomment it. Let's remove this line~
Comment on #188 script to fetch google analytics stats
Seems that errors are all unexpected items that should be reported to Rollbar. It would be nice if we send them using `Rollbar.error(err)`.
Comment on #188 script to fetch google analytics stats
I am wondering if <https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Map.html#putAll(java.util.Map)|`putAll()`> is useful here. I also like the current implementation, which explicitly lists all changed fields, this makes modifications crystal clear :gem: .
Thanks! I've added a minor suggestion regarding the use of `isValid`.
Personally I prefer throwing errors with message text for these utility functions. Pros: • removes the need to do `else` block and simplifies code structure a lot. • have the same expressiveness of error message + `isValid` • when it's invalid, only `error` field is relevant. `isValid` is derived from `error`, and `startDate` / `endDate` are simply not applicable. • in this case, an exception with message can fully express the same amount of information. Cons: • `validateOOO` functions do imply returning error instead of throwing them. • may need to rename to something like `assertOOO`
Comment on #188 script to fetch google analytics stats
Another idea that come across my mind is, processing items across batches should be pretty similar to merging different `pagePathLevel2` -- they all memorize the last row and perform update when necessary. This means that in theory we can just implement merging of different `pagePathLevel2`. When batch changes, we should put `prevLastRow` into the "last-row" memory and perform identical logic that handles `pagePathLevel2`. Another way of thinking is that we blur the boundary of "batch" and only work on one row at a time, providing last "row" (no matter it's from same batch or previous batch) during the process.
Comment on #304 Fix 262 rss function
I was thinking if we can directly convert `listQueryVars` to JSON and put them in feed URL. That should simplify `api/articles/[feed]` (because it don't need `getQueryVars`, just `JSON.parse`) and allows greater flexibility for the feed. What do you think <https://github.com/nonumpa|@nonumpa> ?
Comment on #304 Fix 262 rss function
It's brilliant that we can remove default option from `getQueryVars()`. It makes the code easier to read :+1: I am thinking if we can further simplify this to: ``` const listQueryVars = getQueryVars({ // Defaults that can be overridden by query filters: defaultFilters.join(','), orderBy: defaultOrder, // URL query ...query, // Others to join query timeRangeKey, userId: user?.id, }); ```
Review on #304 Fix 262 rss function
Thanks for the fix! I have attached 2 comments at first glance. Will look at other parts tonight.
Comment on #190 Analytics data loader
Elasticsearch <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/query-dsl-range-query.html#ranges-on-dates|range query> actually supports <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/common-options.html#date-math|date math> for date fields. Currently several inputs like `ListArticleFilter.createdAt`, `ListArticleFilter.repliedAt` leverages `TimeRangeInput` to achieve date range inputs that supports date math, which is essentially exposing the whole range query filter and letting Elasticsearch check the format directly. I am thinking if we can do the same for `analyticsLoader`. I am OK with open ends (For example, with `gte` given but no `lte`) query since Elasticsearch always bound its result to a certain <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/search-request-from-size.html|size>.
Review on #190 Analytics data loader
Thanks for implementing the analytics data loader! I have raised some questions on the `analyticsLoaderFactory` and hope we can discuss before merge.
Comment on #190 Analytics data loader
Since the `ID` used for this data loader is not primitive, we may need to define `cacheKeyFn` so that dataloader can tell if any "id"s given are identical. This is useful for caching and batching. Example: <https://github.com/cofacts/rumors-api/blob/master/src/graphql/dataLoaders/docLoaderFactory.js#L21|https://github.com/cofacts/rumors-api/blob/master/src/graphql/dataLoaders/docLoaderFactory.js#L21> Documentation: <https://github.com/graphql/dataloader#new-dataloaderbatchloadfn--options|https://github.com/graphql/dataloader#new-dataloaderbatchloadfn--options>
Comment on #190 Analytics data loader
Elasticsearch body search by <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/search-request-from-size.html|returns 10 results by default>. This may cause this dataloader never return more than 10 results. We may want to relax that using <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/search-request-from-size.html|`size` param>. Personally I think we can just set a reasonable constant here (Maybe 30 / 60 / 90?). I am ok with crazy values as well -- `articleReplyFeedbackLoader` uses 10,000 lol
LGTM. Thanks for the change!
figma <https://www.figma.com/file/YikWPPauMnukDH0U2Nq4YS/Components?node-id=1%3A310|https://www.figma.com/file/YikWPPauMnukDH0U2Nq4YS/Components?node-id=1%3A310> <https://user-images.githubusercontent.com/6376572/89249900-ffbec000-d645-11ea-8880-78fbb9516f8e.gif|desktop-zh> <https://user-images.githubusercontent.com/6376572/89250032-5b894900-d646-11ea-8923-a9c532abda4b.gif|mobile-en> *Subscribe through IFTTT* <https://user-images.githubusercontent.com/6376572/89250041-62b05700-d646-11ea-8b9e-f4023ffc8d20.png|image> Not Ready: 1. Icon (currently download and search from google image) 2. Copy rss feed button is better to popup a tip 'copied to clipboard'.
Comment on #188 script to fetch google analytics stats
• nit: default
Comment on #188 script to fetch google analytics stats
It would be neat if we have comment noting that this block may (implicitly) modify entries that is pushed into `bulkUpdates` in previous loop.
Review on #188 script to fetch google analytics stats
Thanks for the fix. Please allow me to spend more time inspecting test cases. I need more time to verify if `processReport`is calling `upsertDocStats` with correct `bulk` for the given mocked `rows`.
Review on #188 script to fetch google analytics stats
Finished looking at the test cases of `processReport` and `upsertDocStats`. They look great :)
Comment on #188 script to fetch google analytics stats
If we are going to repeat the `source` of the script many many times in a batch, I think we may consider using stored scripts. <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/modules-scripting-using.html#modules-scripting-stored-scripts|https://www.elastic.co/guide/en/elasticsearch/reference/6.8/modules-scripting-using.html#modules-scripting-stored-scripts> We can update the stored script everytime the cron job starts to make sure the script stored in database is the latest one.
From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#Category-%E6%A8%99%E8%A8%98-UI-%E6%AA%A2%E8%A8%8E|20200729> We should hide obsolete categories from users, but leave their articleCategories in tact (or back it up somewhere)
From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#Category-%E6%A8%99%E8%A8%98-UI-%E6%AA%A2%E8%A8%8E|20200729>: The add-category dialog is now too long and requires a lot of scrolling. Solution TBA.
#307 List 25 items per page in list page UIs
From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#%E3%80%8C%E7%AD%89%E4%BD%A0%E4%BE%86%E7%AD%94%E3%80%8D%E7%9A%84%E5%95%8F%E9%A1%8C|20200729> Currently article list page only show 10 item at a time. We should increase the number to 25 to reduce the need to click "Load more".
#308 Show URL title in article list
From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#%E3%80%8C%E7%AD%89%E4%BD%A0%E4%BE%86%E7%AD%94%E3%80%8D%E7%9A%84%E5%95%8F%E9%A1%8C|20200729> We can display URL's title text in article list. Figma: • Desktop <https://www.figma.com/file/DvmAQjMJCncuPORWKnljM1/Cofacts-website-MrOrz-s-copy?node-id=2675%3A221|https://www.figma.com/file/DvmAQjMJCncuPORWKnljM1/Cofacts-website-MrOrz-s-copy?node-id=2675%3A221> • Mobile <https://www.figma.com/file/DvmAQjMJCncuPORWKnljM1/Cofacts-website-MrOrz-s-copy?node-id=2673%3A220|https://www.figma.com/file/DvmAQjMJCncuPORWKnljM1/Cofacts-website-MrOrz-s-copy?node-id=2673%3A220>
#193 Reload preview for logged in users
From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#%E5%A4%A7%E6%9D%BE%E6%AA%A2%E8%A8%8E|20200729> Sometimes our URL scrapper will be temporarily blocked, leaving useless URL previews like <https://cofacts.org/article/39n5zi0t4vlci|this>. During <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#Youtube-%E9%98%BB%E6%93%8B-metadata-scraping|20200729 discussion> we can provide an API for logged in editors to reload broken review. Spec TBA
#309 Show "OK" button in mobile article filter dialog
From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#%E6%9C%AA%E7%AB%9F%E9%A0%85%E7%9B%AE1|20200729>: > 手機上 filter 按 X 會以為是 cancel,實驗之後才發現是按下去就會 apply Figma: "Apply filter" <https://www.figma.com/file/zpD45j8nqDB2XfA6m2QskO/Cofacts-website?node-id=2322%3A0|https://www.figma.com/file/zpD45j8nqDB2XfA6m2QskO/Cofacts-website?node-id=2322%3A0> We can retain the behavior that pressing filter buttons starts filter process immediately (because it's the current behavior and is easier to implement), but add an "OK" button at the bottom of filter dialog so that user can click to close filter dialog.
#310 Reply request submission button jumps around when clicking
*As-is* <https://user-images.githubusercontent.com/108608/89379879-2f92c400-d729-11ea-90f9-bcc7bcea0bbc.gif|reason-submission> 1. After inputting reason, the reason submission button jumps up and cancels the first click 2. During submission, there is no visual feedback indicating that the reply request is being sent *To-be* 1. Don't change reason input height when reason input is blurred 2. Add a loading state (or simply disable the submission button) to reason submission button when it's being submitted to server.
![]()
g0v.hackmd.io
![]()
Comment on #304 Fix 262 rss function
I also wanted to remove the dependency of `ArticlePageLayout.getQueryVars` from `api/articles/[feed]`, but it seems that it's not easy to directly convert `listQueryVars` to querystring. <https://user-images.githubusercontent.com/6376572/89401059-5102a880-d747-11ea-8185-e96c7c51b9c3.png|螢幕快照 2020-08-05 下午6 01 23> I'm afraid that I should write a `listQueryVarsToQuerystring` function and another `getQueryVars` for `api/articles/[feed]`.
![]()
Comment on #304 Fix 262 rss function
I was thinking a direct `JSON.stringify` should do the job :P Is its result too long for an URL?
Comment on #304 Fix 262 rss function
Sorry, I mean querystring, not JSON. Maybe I can encode filter and orderBy like <https://stackoverflow.com/questions/41641426/passing-objects-as-url-parameters-c-sharp|this>.
Comment on #187 Load more reply request at once
Merged because opened for 10+ days without review.
Is this file used anywhere?
Maybe <https://material-ui.com/api/list-item-text/#props|`secondaryTypographyProps`> could help? e.g. ``` <ListItemText primary={t`Email`} secondary={t`Via Feedrabbit`} secondaryTypographyProps={{variant: 'subtitle2', color: 'textSecondary'}} /> ``` Actually I think it's OK if we use Material UI's default style for `<ListItemText>`. The figma link provided by <https://github.com/LucienLee|@LucienLee> is at wireframe granularity, the font size & color is not fine tuned yet. The actual design system used in mockup is defined here: <https://www.figma.com/file/3tGoYDSrqU8Rqmstyn9NMn/Cofacts-UI-Design-Kit?node-id=0%3A5005|https://www.figma.com/file/3tGoYDSrqU8Rqmstyn9NMn/Cofacts-UI-Design-Kit?node-id=0%3A5005> We can go for Material UI's default first, and see if we need any adjustment in the future.
he figma link provided by <https://github.com/LucienLee|@LucienLee> is at wireframe granularity, the font size & color is not fine tuned yet. The actual design system used in mockup is defined here: <https://www.figma.com/file/3tGoYDSrqU8Rqmstyn9NMn/Cofacts-UI-Design-Kit?node-id=0%3A5005|https://www.figma.com/file/3tGoYDSrqU8Rqmstyn9NMn/Cofacts-UI-Design-Kit?node-id=0%3A5005> I suggest we use `<Typography variant="caption">` for all `<CustomTypography variant="subtitle2">`, and use `<Typography variant="body2">` for all `<CustomTypography variant="subtitle1">`.
The UI is beautiful <3 thanks a lot! I have some suggestions regarding using the use of `CustomTypography` and code dependencies.
We may need to add `copy-to-clipboard` to `package.json`, because it's currently a dev dependency. It won't be installed in production environment. I think `copy-to-clipboard`'s API is much simpler than the ClipboardJS we are currently using. Please consider removing `"clipboard": "^2.0.4"` and replace its usage (only in `CopyButton`) with copy-to-clipboard.
Review on #304 Fix 262 rss function
LGTM! It's beautiful. Thanks a million!
Review on #188 script to fetch google analytics stats
LGTM now! <https://github.com/ztsai|@ztsai> would you like to add <https://github.com/cofacts/rumors-api/pull/188#discussion_r465199034|stored script mechanism> in this PR or do you want to do it in another PR as an enhancement item?
Most of the PR looks good! Some final comments on `<IFTTTItem>` here.
It's not recommended to use `<table>` for something that is not a data table. I suggest using flexbox for this particular UI, which should yield much simpler DOM and CSS style.
If we use <https://material-ui.com/components/dialogs/|`Dialog`> instead of its underlying component <https://material-ui.com/components/modal/|`Modal`>, we would not need to define paper style and positioning here. Dialogs are not necessary used with `DialogTitle`, `DialogContent` etc, it can be used alone, as shown by `ListPageControls`' `Filters`.
g0v.hackmd.io
![]()
#311 ArticleReplyFeedbackControl revamp
This PR is a revamp to this component, including its popover: <https://user-images.githubusercontent.com/108608/89732120-487bdc00-da7f-11ea-9af3-06cdb1f5c13e.png|image> • Rename `ReplyFeedback` to `ArticleReplyFeedbackControl`.
![]()
#312 Upgrade to Apollo Client 3
Apollo Client 3 stable is <https://www.apollographql.com/blog/announcing-the-release-of-apollo-client-3-0/|released>. This PR <https://www.apollographql.com/docs/react/migrating/apollo-client-3-migration/|migrates Apollo Client to 3> and <https://github.com/adamsoffer/next-apollo/blob/master/CHANGELOG.md|next-apollo to latest>. `dataIdFromObject` we are currently using is deprecated in Apollo Client 3; however, in this PR I leave this intact because there are already too much code change.
Comment on #312 Upgrade to Apollo Client 3
Leaving out `{ssr: true}` in `withApollo` here will cause next.js's `useRouter` returning `{}` for `router.query` on server-side rendering & first render on client, because next.js will think the page <https://nextjs.org/docs/api-reference/next/router#router-object|does not meet data fetching requirements>. It's valid for search page because this page is not meant to be accessed directly nor by search engines.
Comment on #188 script to fetch google analytics stats
Thanks! I'll add that in a separate PR as discussed offline
follow up PR for <https://github.com/cofacts/rumors-api/pull/188#discussion_r465199034|stored script mechanism> as discussed in <https://github.com/cofacts/rumors-api/pull/188|#188>
<https://coveralls.io/builds/32649138|Coverage Status> Coverage increased (+0.5%) to 88.065% when pulling *<https://github.com/cofacts/rumors-api/commit/d2c3cd52b7a7cf5759f6ddcfbed3505328396c9c|d2c3cd5> on loadScript* into *<https://github.com/cofacts/rumors-api/commit/2ee6cc9cc8c654b25e2eaae2aa7b7558a7f1d9ee|2ee6cc9> on master*.
Comment on #126 Update all dependencies
*:warning: Artifact update problem* Renovate failed to update an artifact related to this branch. You probably do not want to merge this PR as-is. :recycle: Renovate will retry this branch, including artifacts, only when one of the following happens: • any of the package files in this branch needs updating, or • the branch becomes conflicted, or • you check the rebase/retry checkbox if found above, or • you rename this PR's title to start with "rebase!" to trigger it manually The artifact failure details are included below: *File name: package-lock.json*