Review on #231 On-boarding tutorial
The unit tests are extremely helpful when I read through the code. Thanks for providing the thorough tests cases! I have some comments regarding wording and how `replies` array can be constructed.
Comment on #231 On-boarding tutorial
Please replace `"send your message to our chatroom"` with `"Send messages to chats"` to match <https://github.com/cofacts/rumors-line-bot/issues/216#issuecomment-718374931|LINE UI> and also update the translate file key, thanks!
Comment on #231 On-boarding tutorial
It seems that `createPermissionSetupDialog` does not return a promise, thus no need to `await` here? Please check if all `await` in this method are necessary. If all of them are not, we may remove `async` for `tutorial()` as well :+1: Another thing is, no matter we need `await` or not, the construction of `replies` can be simplied using object spread operator: ``` replies = [ { type: 'text', text: replyProvidePermissionSetup, }, { ...await createPermissionSetupDialog(askForPermissionSetup), quickReply: {...} }, ] ``` The pattern above can be applied to all other `if` cases and replace all `replies.push()` invocation.
Comment on #231 On-boarding tutorial
Why does serve middleware not work for dev environment?
#4 Remove privacy information from reply reference
As title
Comment on #231 [wip] Cleanuptests
<https://coveralls.io/builds/34667833|Coverage Status> Coverage decreased (-0.03%) to 86.035% when pulling *<https://github.com/cofacts/rumors-api/commit/f4ec4b2c9d52c7fa56533a2a7ef27fb72e2bc441|f4ec4b2> on cleanuptests* into *<https://github.com/cofacts/rumors-api/commit/177693c345bcbbed7e442f1c71fec3bf3a075e0e|177693c> on backendUsers*.
Nit: seems that this file is no longer a graphql mutation and is more like a util function. Maybe we can consider moving it to `graphql/models/User`, or move to `util` (along with all utility functions under `graphql/models/User`). We may <https://github.com/magicmark/jest-how-do-i-mock-x/blob/master/src/function-in-same-module/README.md|need some work around to properly mock these utility in the same file>.
nit: `appUserId` here actually can be db user id or app user id. Maybe we can denote this in the doc string, or even choose to change argument name to `dbOrAppUserId` or just `userId`.
LGTM! Thanks for the fix! Added some nit-picking comments that should not be merge blockers. Feel free to adapt and merge anytime :)
nit: indent XD
It's looking great! Thanks for making the tests more robust!
Comment on #231 On-boarding tutorial
I don't know liff uses proxy to a webpack server for development, just copy and paste the `/liff` part to apply `/static` at that time.
Comment on #231 On-boarding tutorial
Text will ellipsis before assigning to altText, so the snapshot looks a little weird.
Review on #231 On-boarding tutorial
LGTM! Let's merge!
HackMD
Cofacts Toolkit ======= ### Introduction - [Cofacts Tutorial](<https://g0v.hackmd.io/Waz-B9sORSOPLp>
#233 Update tutorial wording and compress images
Details see : <https://g0v.hackmd.io/Sl_84QO8TQ6WKKI0bT4pYw#%E6%9C%AA%E7%AB%9F%E9%A0%85%E7%9B%AE|https://g0v.hackmd.io/Sl_84QO8TQ6WKKI0bT4pYw#%E6%9C%AA%E7%AB%9F%E9%A0%85%E7%9B%AE>
Comment on #233 Update tutorial wording and compress images
*Pull Request Test Coverage Report for <https://coveralls.io/builds/34710866|Build 1202>* • *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 *85.94%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*
Review on #233 Update tutorial wording and compress images
LGTM, thanks for the timely fix!
Fix wrong image order caused by commit `Compress tutorial images`. bug snapshot <https://user-images.githubusercontent.com/6376572/98199754-e855e280-1f66-11eb-8364-3d8e48cd7d4f.png|Screenshot_20201105-124700>
![]()
Comment on #234 Fix tutorial image order
*Pull Request Test Coverage Report for <https://coveralls.io/builds/34729643|Build 1207>* • *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 *85.94%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*
Review on #234 Fix tutorial image order
Thanks for the fix! If applicable, I suggest adding cache busting argument (like `${RUMORS_LINE_BOT_URL}/xxx.png?v=20201105`) to make sure users get the latest image after deploy.
Thanks for the refactor! Let's merge this. I will try deploy it to staging before wednesday, along with data migration.
數位時代
我們在網路的平台資本主義世界裡行走、生存,往往只看到樹木,看不到森林。
![]()
Comment on #234 Fix tutorial image order and handle the case RUMORS_LINE_BOT_URL undefined
I suggest we keep `VERSION` as a variable in `tutorial.js` because • If we change image in the future, it's natural that we go to tutorial.js and make changes there • Since image updates are under source control, so should its `VERSION`. The change of image file and the change of `VERSION` variable should be updated together in a PR.
Review on #234 Fix tutorial image order and handle the case RUMORS_LINE_BOT_URL undefined
Thanks for adding the cache-busting mechanism! I have some suggestion regarding where we put the cache busting variable.
Review on #234 Fix tutorial image order and handle the case RUMORS_LINE_BOT_URL undefined
Thanks for the fix! Let's <https://github.githubassets.com/images/icons/emoji/shipit.png|:shipit:>
![]()
Speaker Deck
LINE API Platform Update 202011
LINE API Platform Update 2020/11 by NiJia Lin @ Chatbot community #25 <https://chatbots.kktix.cc/events/meetup-025>
![]()
Stack impact was introduced to mitigate CPU issue: <https://github.com/cofacts/rumors-site/pull/189|#189> However after it's added, we did not get meaningful insight from it. On the other hand, it requires native extension when installing the project and hence stops some of our contributor in g0v hackathon from contributing. I suggest we remove stackimpact from rumors-site bundle.
Google Developers
Tell Google about localized versions of your page
![]()
Google Developers
Consolidate duplicate URLs | Google Search Central | Google Developers
![]()
Review on #354 Remove stackimpact
LGTM!
Google Developers
Managing multi-regional and multilingual sites | Google Search Central
![]()
#3 Apply data grid to thank-you editor
• Better appearance for the reply list page <https://cofacts.github.io/community-builder/#/editorworks|https://cofacts.github.io/community-builder/#/editorworks> • Add date filter <https://user-images.githubusercontent.com/108608/99219781-59738080-2818-11eb-8030-ffc33555f67e.png|image>
![]()
Fixes <https://github.com/cofacts/rumors-line-bot/issues/222|#222> dialogflow intent design <https://g0v.hackmd.io/kWuTnoC1TG-MONPhxp8rGQ#2020-Q4--2021-Q1-%E9%80%B2%E5%BA%A6%E7%A2%BA%E8%AA%8D|discussion>
Comment on #235 Feature/dialogflow
*Pull Request Test Coverage Report for <https://coveralls.io/builds/35015547|Build 1216>* • *21* of *21* *(100.0%)* changed or added relevant lines in *2* files are covered. • No unchanged relevant lines lost coverage. • Overall coverage increased (+*0.3%*) to *86.38%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*
Comment on #233 ctx userId should be dbUserId
<https://coveralls.io/builds/35051417|Coverage Status> Coverage increased (+0.06%) to 86.003% when pulling *<https://github.com/cofacts/rumors-api/commit/d09fdf58809aabe60a5f86155b1c88ed3c43581b|d09fdf5> on ctxUserId* into *<https://github.com/cofacts/rumors-api/commit/2c0769ea2273fe96efee24006607e8f9bf934b95|2c0769e> on master*.
Review on #52 add slug & bio fields for user
Thanks for the update!
Comment on #52 add slug & bio fields for user
Thanks for updating the typo! I found that the text is a bit outdated as well. Suggested change
Comment on #232 [wip] user profile related api changes
<https://coveralls.io/builds/35067240|Coverage Status> Coverage increased (+0.4%) to 86.354% when pulling *<https://github.com/cofacts/rumors-api/commit/af2a790f0aff540478d4110defd2f4231fd91bd1|af2a790> on userProfile* into *<https://github.com/cofacts/rumors-api/commit/2c0769ea2273fe96efee24006607e8f9bf934b95|2c0769e> on master*.