cofacts

HackMD

在 LINE Chatbot 中串接 Google Analytics 的經驗分享 - HackMD

在 LINE Chatbot 中串接 Google Analytics 的經驗分享

#189 Date util

small refactor, moving date range helper to util to be reused in analytics dataloader

#190 Analytics data loader

depending on <https://github.com/cofacts/rumors-api/pull/188|#188> and <https://github.com/cofacts/rumors-api/pull/189|#189>

Comment on #189 Date util

<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*.

#191 Article stats

Comment on #191 Article stats

<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: .

Review on #189 Date util

Thanks! I've added a minor suggestion regarding the use of `isValid`.

Comment on #189 Date util

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.

116155217_350793832748131_7278387044960444471_n.jpg

116207176_1231006320590790_4735981624252461450_n.jpg

116212778_216506436350194_8151330362891010884_n.jpg

116235900_378582476456256_530460603154245394_n.jpg

116800306_291163598653757_6610934379152638743_n.jpg

117068519_327122458671111_816188199161369806_n.jpg

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

Review on #189 Date util

LGTM. Thanks for the change!

#305 RSS UI

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.

#192 Hide obsolete categories

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)

#306 Improve category dialog

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>: &gt; 手機上 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

20200805 會議記錄 - HackMD

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

#164 replyRequests should return more than 10 entries

#187 Load more reply request at once

Comment on #187 Load more reply request at once

Merged because opened for 10+ days without review.

Comment on #305 RSS UI

Is this file used anywhere?

Comment on #305 RSS UI

Maybe <https://material-ui.com/api/list-item-text/#props|`secondaryTypographyProps`> could help? e.g. ``` &lt;ListItemText primary={t`Email`} secondary={t`Via Feedrabbit`} secondaryTypographyProps={{variant: 'subtitle2', color: 'textSecondary'}} /&gt; ``` Actually I think it's OK if we use Material UI's default style for `&lt;ListItemText&gt;`. The figma link provided by <https://github.com/LucienLee|@LucienLee> is at wireframe granularity, the font size &amp; 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.

Comment on #305 RSS UI

he figma link provided by <https://github.com/LucienLee|@LucienLee> is at wireframe granularity, the font size &amp; 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 `&lt;Typography variant="caption"&gt;` for all `&lt;CustomTypography variant="subtitle2"&gt;`, and use `&lt;Typography variant="body2"&gt;` for all `&lt;CustomTypography variant="subtitle1"&gt;`.

Review on #305 RSS UI

The UI is beautiful &lt;3 thanks a lot! I have some suggestions regarding using the use of `CustomTypography` and code dependencies.

Comment on #305 RSS UI

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.

DSC-TW/SC-cofacts-extension

Review on #304 Fix 262 rss function

LGTM! It's beautiful. Thanks a million!

#262 Fix RSS function

#304 Fix 262 rss function

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?

Review on #305 RSS UI

Most of the PR looks good! Some final comments on `&lt;IFTTTItem&gt;` here.

Comment on #305 RSS UI

It's not recommended to use `&lt;table&gt;` 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.

Comment on #305 RSS UI

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

關於 / 教學 - HackMD

#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 &amp; 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.

Review on #311 ArticleReplyFeedbackControl revamp

#311 ArticleReplyFeedbackControl revamp

Comment on #188 script to fetch google analytics stats

Thanks! I'll add that in a separate PR as discussed offline

#181 Integrate with google analytics reporting API

#188 script to fetch google analytics stats

#189 Date util

#194 Load script

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>

Comment on #194 Load script

<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*

Review on #190 Analytics data loader

LGTM! Thanks for the code change :rocket:

Review on #191 Article stats

LGTM! Lets <https://github.githubassets.com/images/icons/emoji/shipit.png|:shipit:>

Review on #194 Load script

LGTM! :+1: :+1: :+1:

#214 Don't ask for article source in LIFF anymore

Spec TBD • 提案＆過去討論整理：<https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA?view#chatbot-%E4%B8%8D%E5%95%8F%E4%BE%86%E6%BA%90|https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA?view#chatbot-%E4%B8%8D%E5%95%8F%E4%BE%86%E6%BA%90> • 方案選擇、文案討論：<https://g0v.hackmd.io/@mrorz/cofacts-meeting-notes/%2FrEtx-47bRHeW2aM05DINDQ|https://g0v.hackmd.io/@mrorz/cofacts-meeting-notes/%2FrEtx-47bRHeW2aM05DINDQ>

#182 Implement data loader for analytics

#190 Analytics data loader

#183 Implement API endpoint to query for analytics stats

#191 Article stats

#194 Load script

Comment on #305 RSS UI

It seems that doesn't need to set success at next tick

#215 Fix issue, node process won't exit automatically when running notify

Disconnect redis and mongodb at the end of `scanRepliesAndNotify`.

Comment on #215 Fix issue, node process won't exit automatically when running notify

*Pull Request Test Coverage Report for <https://coveralls.io/builds/32706266|Build 1098>* • *2* of *2* *(100.0%)* changed or added relevant lines in *1* file are covered. • No unchanged relevant lines lost coverage. • Overall coverage increased (+*0.03%*) to *84.292%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*

Comment on #305 RSS UI

agree

Review on #305 RSS UI

Review on #215 Fix issue, node process won't exit automatically when running notify

LGTM! Will test it out when I have access to latop.

Review on #215 Fix issue, node process won't exit automatically when running notify

It is really working :+1: I suggest adding `.catch((e) =&gt; {console.error(e); process.exit(1)})` to the end of `scanRepliesAndNotify()` so that errors can be seen when there are exceptions.

#215 Fix issue, node process won't exit automatically when running notify

#45 [WIP] Add migration script for analytics index

1. Add migration script for `analytics` index so that we can add the new index mapping to existing database 2. Make `db/loadSchema.js` exit with non-zero return code so that Travis can capture the error when schema is invalid Need <https://github.com/ztsai|@ztsai> 's help looking at errors inserting index to database :pray:

#195 Update rumors-db with analytics index mappings

• Update `test/rumors-db` submodule revision • Update unit test snapshot accordingly • Mostly about index name change (alias --&gt; real index name) • Seems that elasticsearch is returning different order for indexes that has mappings. Not sure why, but I'll just accept that :P

Comment on #195 Update rumors-db with analytics index mappings

<https://coveralls.io/builds/32752094|Coverage Status> Coverage remained the same at 88.265% when pulling *<https://github.com/cofacts/rumors-api/commit/3bb554cb0d76052fc62e40b1b2ed054aec82ecdc|3bb554c> on rumors-db-update* into *<https://github.com/cofacts/rumors-api/commit/a0a43a18523b0a595b15b48bef9a03c4102b8d73|a0a43a1> on master*.

Review on #195 Update rumors-db with analytics index mappings

For the unit tests, now that I think about it, the returned order is more or less random since all _score values are 1. It would probably be better to provide sorting options in the unit tests since we are doing snapshot matches and the indeterminacy of returned order makes them unreliable. But it's outside of the scope of this PR, I can do that in a separate PR.