Skip to content
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.

Sign up for GitHub

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: contextualize email notifications #327

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Notifications: contextualize email notifications #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 29 additions & 26 deletions src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import GlobalErrorHandler from './GlobalErrorHandler'
import MainView from './components/MainView'
import OnboardingLoader from './components/OnboardingLoader'
import EmailNotificationsLoader from './components/EmailNotificationsLoader'
import EmailNotificationsProvider from './components/EmailNotificationsProvider'
import RequestPanel from './components/RequestPanel/RequestPanel'
import Routes from './Routes'
import { ActivityProvider } from './providers/ActivityProvider'
Expand All @@ -20,32 +21,34 @@ function App() {
<WalletProvider>
<BrowserRouter>
<ActivityProvider>
<Main
assetsUrl="/aragon-ui/"
layout={false}
scrollView={false}
theme={theme}
>
<GlobalErrorHandler>
<ToastHub threshold={1} timeout={1500}>
<CourtConfigProvider>
<CourtClockProvider>
<RequestQueueProvider>
<MainView>
<OnboardingLoader>
<EmailNotificationsLoader />
<AppLoader>
<Routes />
</AppLoader>
</OnboardingLoader>
<RequestPanel />
</MainView>
</RequestQueueProvider>
</CourtClockProvider>
</CourtConfigProvider>
</ToastHub>
</GlobalErrorHandler>
</Main>
<EmailNotificationsProvider>
<Main
assetsUrl="/aragon-ui/"
layout={false}
scrollView={false}
theme={theme}
>
<GlobalErrorHandler>
<ToastHub threshold={1} timeout={1500}>
<CourtConfigProvider>
<CourtClockProvider>
<RequestQueueProvider>
<MainView>
<OnboardingLoader>
<EmailNotificationsLoader />
<AppLoader>
<Routes />
</AppLoader>
</OnboardingLoader>
<RequestPanel />
</MainView>
</RequestQueueProvider>
</CourtClockProvider>
</CourtConfigProvider>
</ToastHub>
</GlobalErrorHandler>
</Main>
</EmailNotificationsProvider>
</ActivityProvider>
</BrowserRouter>
</WalletProvider>
Expand Down
142 changes: 142 additions & 0 deletions src/components/EmailNotifications/EmailNotificationsProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
import React, {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: we may want to rename this file to be EmailNotificationsApiProvider for clarity.

useCallback,
useContext,
useEffect,
useRef,
useState,
} from 'react'
import PropTypes from 'prop-types'
import { useWallet } from '../../providers/Wallet'
import {
getJurorEmail,
getSubscriptionDetails,
subscribeExistingEmail,
subscribeToNotifications,
} from '../../services/servicesRequests'

const EmailNotificationsContext = React.createContext()

function EmailNotificationsProvider({ children }) {
const { account } = useWallet()

const asyncCancelled = useRef(false)
const [email, setEmail] = useState(null)
const [needsSignature, setNeedsSignature] = useState(false)
const [subscriptionDetails, setSubscriptionDetails] = useState({
error: null,
fetching: false,
})

const handleOnSubscribe = useCallback(
async email => {
const response = await subscribeToNotifications(account, email)

if (response.error && !response.needsSignature) {
return response.error
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this, we may want to throw the error instead (and perhaps all the requests should be throwing those errors, similar to https://github.com/aragon/aragon/blob/master/src/components/GlobalPreferences/Notifications/notification-service-api.js#L60).

}

if (!asyncCancelled.current) {
setEmail(email)
if (needsSignature !== Boolean(response.needsSignature)) {
setNeedsSignature(!needsSignature)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to figure out how are you thinking to handle the action after the signature? i mean after the signature is successful we need to call again the subscribeToNotifications again.
And the other question that i have is : given that here we don't have an useEffect that listen to the needsSignature state, i guess that the plan is to have that useEffect in the manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that here we don't have an useEffect that listen to the needsSignature state, i guess that the plan is to have that useEffect in the manager?

Yes, exactly, that was something I wanted to leave to the state machine manager, given that we could have different machines based on the area (preferences or modal).

Trying to figure out how are you thinking to handle the action after the signature?

Similar to above, I was hoping the manager would handle this. These functions just attempt to "optimistically" call the API, and if it fails, it'll let us know either with the error or a state change.

}
}
},
[account, needsSignature]
)

const handleOnSubscribeExistingEmail = useCallback(async () => {
const response = await subscribeExistingEmail(account)

if (response.error && !response.needsSignature) {
return response.error
}

if (!asyncCancelled.current) {
if (email !== response.email) {
setEmail(response.email)
if (needsSignature !== Boolean(response.needsSignature)) {
setNeedsSignature(!needsSignature)
}
}
}
}, [account, email, needsSignature])

// Cancel any async requests if this provider is unmounted
useEffect(() => {
return () => {
asyncCancelled.current = true
}
}, [])

// When account connects, fetch their subscription details
useEffect(() => {
if (!account) {
return
}

const fetchSubscriptionDetails = async () => {
const response = await getSubscriptionDetails(account)

if (!asyncCancelled.current) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the same asyncCancelled ref here and after, but thinking about it, useEffects() should contain their own local cancellation.

setSubscriptionDetails({
...response,
fetching: false,
})
}
}

setSubscriptionDetails(subscriptionDetails => ({
...subscriptionDetails,
error: null,
fetching: true,
}))
fetchSubscriptionDetails()
}, [account])

// Once we know the account has an associated email, fetch its email
useEffect(() => {
if (!subscriptionDetails.emailExists) {
return
}

const getEmail = async () => {
const { needsSignature, email } = await getJurorEmail(account)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving a second thought to this, i think that here we are going to be fetching the juror email every time an account is connected, and when the cookie has expired we are going to ask the juror to sign a message just for connecting his account and not sure if that is what we want.
The way i see this is that we need to do that only when we are subscribing to notifications through the modal or entering the global preferences not every time the juror connect his account to interact with the dashboard.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Yes, it sounds like we should do this in a callback instead.


if (!asyncCancelled.current) {
if (email) {
setEmail(email)
}
if (needsSignature) {
setNeedsSignature(true)
}
}
}

getEmail()
}, [account, subscriptionDetails.emailExists])

return (
<EmailNotificationsContext.Provider
value={{
email,
handleOnSubscribe,
handleOnSubscribeExistingEmail,
needsSignature,
subscriptionDetails,
}}
>
{children}
</EmailNotificationsContext.Provider>
)
}

EmailNotificationsProvider.propTypes = {
children: PropTypes.node,
}

function useEmailNotifications() {
return useContext(EmailNotificationsContext)
}

export { EmailNotificationsProvider, useEmailNotifications }