-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Notifications service: first batch of notifications #165
Conversation
* moved emails from court-dashboard repo * Emails: add `email-verification-reminder` and `missed-reveal` templates (#160) * Emails: add missed-reveal template * Emails: update email verification template to be have more template parameters * Emails: update missed-vote to missed-action with additional templating * Revert "Emails: update missed-vote to missed-action with additional templating" This reverts commit 64f84f1. * Revert "Emails: update email verification template to be have more template parameters" This reverts commit ce13ea2. * Emails: add email-verification-reminder template * email-verification typo * text changes to verification reminder * changed wording to emailPreferencesUrl * Added subject line required for email sync #161 Co-authored-by: Martynas Prokopas <[email protected]> * Emails: automatic sync (#161) * added sync:templates and sync:assets * added ci/cd * updated README * Apply suggestions from code review Co-authored-by: Brett Sun <[email protected]> * updated readme * minor fixes * Update emails/package.json Co-authored-by: Brett Sun <[email protected]> * changed to only sync on mainnet * Apply suggestions from code review Co-authored-by: Facu Spagnuolo <[email protected]> * added staging postmark server for development * ci/cd test * test2 * reverted after test Co-authored-by: Brett Sun <[email protected]> Co-authored-by: Facu Spagnuolo <[email protected]> Co-authored-by: Brett Sun <[email protected]> Co-authored-by: Facu Spagnuolo <[email protected]>
Co-authored-by: Facu Spagnuolo <[email protected]>
packages/services/src/models/notification-scanners/JurorDrafted.js
Outdated
Show resolved
Hide resolved
* notifications scaffolding * typo * added not nullabel * added last notification date check * added notification types table * split into notification scanner and sender * notification service prototype * renamed test-server to test since we are testing services as well * changed test folder * added email template for subscription reminder * Apply suggestions from code review Co-authored-by: Facu Spagnuolo <[email protected]> * minor suggested fixes * added scan check * abstracted scanner check * renamed scanner checkUser to shouldNotifyUser * added some user abstractions * typo typo typo * added more reliable test script * improved test errors * minor test fixes * switched to logger error instead of throw * removed $ from objection methods * removed query() from some base methods * moved logs before cors * tidying up test file * removed subscription reminder for anj registrations * send verification token for anj registrations * added reminder email instructions Co-authored-by: Facu Spagnuolo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I think we can start testing this in Rinkeby.
Let's fix the URL of the draft template, let's make sure to point to the right court dashboard env
@@ -1,6 +1,7 @@ | |||
import emailClient from '@aragonone/court-backend-shared/helpers/email-client' | |||
import { UserNotification } from '@aragonone/court-backend-server/build/models/objection' | |||
import * as notificationScanners from '../models/notification-scanners' | |||
import { accountData } from '../../../../emails/mock-utils' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we rename this to utils
or helpers
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to helpers
packages/services/src/models/notification-scanners/JurorDrafted.js
Outdated
Show resolved
Hide resolved
fc7e1e5
to
ecb20cf
Compare
* added DueTasks * added AppealsOpened * added DisputeRuled * added MissedVote MissedReveal * edited missed vote/reveal email links * parameterized lockedAnjBalanceUrl * added final ruling word * simplified outcomes word * updated due tasks mock time format * lowercased am/pm * minor fix missed vote/reveal * minor fix missed vote * Shared: Add court wrapper config getter (#170) * shared: add court wrapper config getter * Shared: Rename court config getter * cli: fix config setter command * updated DisputeRuled with ruledAt field * changed adjudiationRound time checks to termId * shared: add court start time getter * removed logger padding for longer worker names * fixed due date with data from the blockchain * added bn casting * Notifications service: Notify unverified anj registrations (#171) * added notify-new-anj-registrations * minor template fixes * added ethereum instruction * added notifications setings link in the footer * removed trim * typo * changed anj function name * minor script fixes and comments Co-authored-by: Facu Spagnuolo <[email protected]> Co-authored-by: Facu Spagnuolo <[email protected]>
@@ -0,0 +1,48 @@ | |||
import NotificationScannerBaseModel from './NotificationScannerBaseModel' | |||
import Network from '@aragonone/court-backend-server/build/web3/Network' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should lift the common dependencies of the services and server out?
In particular, I'm wondering if we should have something like a db
package that contains all the migrations, configuration, and models. It would be really weird if the server and services each had separate versions of the models.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating issue #196
So the flow I used for the test is just basically test the query manually on https://thegraph.com/explorer/subgraph/aragon/aragon-court
and then replace
Network.query()
with the structured result.You are probably manually testing those queries anyway before writing code so I think this is a good flow to provide some sort of integration testing.