feat(notifier): adds notification channels #842

vjgene · 2021-05-10T18:21:30Z

Adds Notification channels section in notifier buy checkout page

itofarina

Overall looks good. Just please consider those 2 comment:

Template the component we wanted to reuse instead of copying it
Use react-hook-form library for data input

itofarina · 2021-05-11T13:42:23Z

src/components/organisms/notifier/NotificationChannel.tsx

+          <Grid xs={6} item style={{ display: 'flex', justifyContent: 'space-around', alignItems: 'center' }}>
+            {shortenString(channel.destination, 30, 25)}
+          </Grid>
+          <Grid xs={3} item>
+            <RemoveButton
+              style={{
+                maxWidth: '20px', maxHeight: '20px', minWidth: '20px', minHeight: '20px',
+              }}
+              id={channel.type}
+              handleSelect={onRemoveClick}
+            />
+          </Grid>


Please, prefer classes instead of inline styles. See this SO question for more info

itofarina · 2021-05-11T13:45:06Z

src/components/organisms/notifier/NotificationChannel.tsx

+        <Grid
+          container
+          alignItems="center"
+          className={classes.innerContainer}
+          spacing={1}
+        >


If this is the same row styles we were talking about, I think it would be best to template it. That way we could reuse it here and if we want to change it in the future we would only need to do the change in one place.

itofarina · 2021-05-11T13:49:29Z

src/components/organisms/notifier/NotificationChannelsList.tsx

+    destination: channel.destination,
+  }))
+
+  return (


I know we have a lot of Grid components acting as forms, but we are trying to migrate them to https://react-hook-form.com/. With that we don't need to create validations for emails for example. It also improves UX.
See #710 and #709 for reference

jurajpiar

A couple more changes from me too, please.

jurajpiar · 2021-05-11T17:07:52Z

src/models/marketItems/NotifierItem.ts

+export enum NotifierChannelType {
+   API = 'Enter api destination',
+   EMAIL = 'Enter your email'
+}


Prefer using const assertion to enums, unless you'd benefit from bidirectional resolution. See https://tinyurl.com/enumconst

src/utils/validationUtils.ts

jurajpiar · 2021-05-11T17:15:54Z

src/utils/validationUtils.ts

+export const validateEmail = (emails: string): boolean => {
+  const re = /^(([^<>()[\]\\.,;:\s@"]+(\.[^<>()[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/
+  let valid = true
+  emails.split(';').forEach((email) => {
+    if (re.test(String(email).toLowerCase()) === false) valid = false
+  })
+  return valid
+}


let's not validate emails in the UI.

expensive

doesn't actually validate the email address

once we do start using email in channels, we should send a verification email instead.

jurajpiar · 2021-05-11T17:16:44Z

src/components/pages/notifier/buy/NotifierOffersSelectedPage.tsx

+      <br />
+      <br />


Use grid to position things on page instead.

jurajpiar · 2021-05-11T17:17:03Z

src/components/organisms/notifier/NotificationChannelsList.tsx

+      <br />
+      <br />


don't need this. use grid and/or css

jurajpiar · 2021-05-11T17:20:01Z

src/components/organisms/notifier/NotificationChannelsList.tsx

+    if (!validate(notifierChannel)) {
+      return
+    }
+    const index = addedChannels.findIndex((ch) => ch.type === notifierChannel.type)


please use meaningful variable names (yea, even for iterator arguments)

jurajpiar · 2021-05-11T17:20:54Z

src/components/organisms/notifier/NotificationChannelsList.tsx

+    if (!validate(notifierChannel)) {
+      return


use the forms dependency for validation, if needed. (would not worry about that right now tbh tho)

jurajpiar · 2021-05-11T17:21:02Z

src/components/organisms/notifier/NotificationChannelsList.tsx

+  if (!channels) {
+    return <Grid />
+  }
+  const availableChannels: Array<string> = channels.map((ch) => (NotifierChannelType[ch] ? ch : ''))


please use meaningful variable names (yea, even for iterator arguments)

jurajpiar · 2021-05-11T17:22:20Z

src/components/organisms/notifier/NotificationChannelsList.tsx

+  const [destination, setDestination] = useState<string>('')
+  const handleChange = (e) => {
+    setChannel({
+      type: e.target.value as string,


destructure {target} or {target: {value}}

src/components/organisms/notifier/NotificationChannelsList.tsx

jurajpiar

A couple more bits & bobs.

jurajpiar · 2021-05-13T07:52:55Z

src/utils/validationUtils.ts

+const validateURL = (_url: string): boolean => {
+  let url
+  try {
+    url = new URL(_url)
+  } catch (_) {
+    return false
+  }
+  return url.protocol === 'http:' || url.protocol === 'https:'


I think it would be better to use a constant for the valid protocols. Also, we're trying to keep the use of mutable variables close to none. Another point is to try not to use arg names with underscore prefix. In fact I'm surprised eslint didn't complain about that. I suggest following structure:

Suggested change

const validateURL = (_url: string): boolean => {

let url

try {

url = new URL(_url)

} catch (_) {

return false

}

return url.protocol === 'http:' || url.protocol === 'https:'

const EVENT_API_AVAILABLE_PROTOCOLS = [ 'http:', 'https:'] // we could potentially make this configurable, for sure we should move it somewhere more in scope of the notifier service

const validateURLProtocol = (url: string, validProtocols: Array<string>): boolean => {

try {

const { protocol } = new URL(url)

return validProtocols.includes(protocol);

} catch { // note that catch in js doesn't need arguments

return false

}

}

jurajpiar · 2021-05-13T08:07:35Z

src/components/organisms/notifier/NotificationChannelsList.tsx

+  if (!channels) {
+    return <Grid />
+  }
+  const availableChannels: Array<string> = channels.map((channel) => (notifierChannelType[channel] ? channel : ''))


I get that you want to restrict the use to only those currently supported channels with this. If so, then we should make this either configurable, or a constant somewhere. However, one that represents this, like const EVENT_AVAILABLE_CHANNELS = ['API'] (currently just that API I think. Is that right @artem-brazhnikov?).
Another thing here is that maybe use .filter instead of mapping to potentially empty string.

jurajpiar · 2021-05-13T08:14:55Z

src/components/organisms/notifier/NotificationChannelCreate.tsx

+import Typography from '@material-ui/core/Typography'
+import validateURL from 'utils/validationUtils'
+
+export interface NotificationChannelCreateProps {


As not used outside just call it Props

jurajpiar · 2021-05-13T08:21:42Z

src/models/marketItems/NotifierItem.ts

@@ -29,3 +29,13 @@ export type NotifierSubscriptionItem = Item & Omit<SubscriptionDTO, 'hash' | 'pr
  price: Big
  token: SupportedToken
 }
+
+export const notifierChannelType = {


Sorry didn't pick up on this before, but let's call this notifierChannelPlaceholder and move it to the src/components/organisms/notifier/NotificationChannelCreate.tsx as it is not needed for any other areas of the UI.

jurajpiar · 2021-05-13T13:03:58Z

src/components/organisms/notifier/NotificationChannelCreate.tsx

+          }
+      </Grid>
+      <Grid item xs={3} md={3}>
+        <SelectRowButton id="add-channel" handleSelect={handleSubmit(() => channelAdd(notifierChannel))} size="large" variant="outlined">Add Channel</SelectRowButton>


split to multiple lines

jurajpiar · 2021-05-13T13:06:33Z

src/components/organisms/notifier/NotificationChannelsList.tsx

+}))
+
+type Props = {
+    channels: string[] | undefined


channels?: Array< SupportedEventChannel>

jurajpiar · 2021-05-13T13:07:26Z

src/components/organisms/notifier/NotificationChannelsList.tsx

+  if (!channels) {
+    return <Grid />
+  }
+  const availableChannels: Array<string> = channels.filter((channel) => SUPPORTED_EVENT_CHANNELS.indexOf(channel as SupportedEventChannel) !== -1)


multiple lines

const availableChannels: Array< SupportedEventChannel>

SUPPORTED_EVENT_CHANNELS.includes(channel)

jurajpiar · 2021-05-13T13:11:55Z

src/utils/validationUtils.ts

@@ -0,0 +1,10 @@
+export const EVENT_API_AVAILABLE_PROTOCOLS = ['http:', 'https:']


this should be in the notifier config now. (also could you please rename it to match the other const please? my bad, sorry)

vjgene added 2 commits May 7, 2021 17:06

feat(notifier): adds checkout page

89f7da4

Merge branch 'develop' into feature/addchannels

a32e88d

vjgene requested a review from jurajpiar as a code owner May 10, 2021 18:21

vjgene requested review from 0xartem and itofarina May 10, 2021 18:22

vjgene assigned 0xartem and jurajpiar May 10, 2021

feat(notifier): add notification channels

551c7e7

vjgene force-pushed the feature/addchannels branch from ea40d49 to 551c7e7 Compare May 11, 2021 11:34

itofarina reviewed May 11, 2021

View reviewed changes

jurajpiar suggested changes May 11, 2021

View reviewed changes

0xartem and others added 3 commits May 12, 2021 23:08

chore: add rskdevnet contracts (#843)

78a7315

feat(notifier): adds notification channels

941d8ce

Merge branch 'develop' into feature/addchannels

2bbd003

vjgene force-pushed the feature/addchannels branch from 2fb2a8f to 2bbd003 Compare May 12, 2021 17:41

jurajpiar suggested changes May 13, 2021

View reviewed changes

refactor(notifier): adds SupportedEventChannel type

7a7c686

vjgene force-pushed the feature/addchannels branch from 9d5d06e to 7a7c686 Compare May 13, 2021 15:01

Merge branch 'develop' into feature/addchannels

f91895e

jurajpiar approved these changes May 14, 2021

View reviewed changes

vjgene merged commit 70126d0 into develop May 14, 2021

vjgene deleted the feature/addchannels branch May 14, 2021 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(notifier): adds notification channels #842

feat(notifier): adds notification channels #842

vjgene commented May 10, 2021

itofarina left a comment

itofarina May 11, 2021

itofarina May 11, 2021

itofarina May 11, 2021

jurajpiar left a comment

jurajpiar May 11, 2021

jurajpiar May 11, 2021

jurajpiar May 11, 2021

jurajpiar May 11, 2021 •

edited

Loading

jurajpiar May 11, 2021

jurajpiar May 11, 2021

jurajpiar May 11, 2021

jurajpiar May 11, 2021

jurajpiar left a comment

jurajpiar May 13, 2021

jurajpiar May 13, 2021

jurajpiar May 13, 2021

jurajpiar May 13, 2021

jurajpiar May 13, 2021

jurajpiar May 13, 2021

jurajpiar May 13, 2021

jurajpiar May 13, 2021

		@@ -0,0 +1,10 @@
		export const EVENT_API_AVAILABLE_PROTOCOLS = ['http:', 'https:']

feat(notifier): adds notification channels #842

feat(notifier): adds notification channels #842

Conversation

vjgene commented May 10, 2021

itofarina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jurajpiar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jurajpiar May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jurajpiar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jurajpiar May 11, 2021 •

edited

Loading