feat(client, db): event url #423

gh-23378 · 2020-10-25T00:17:54Z

I have read Chapter's contributing guidelines.
My pull request has a descriptive title (not a vague title like Update README.md).
My pull request targets the master branch of Chapter.

Closes #381

Added URL field to event model, created migration for new column to be added to event table, updated seeders to generate fake URLs, updated UI to allow read and update on URL.

While working on this I noticed that the form to edit events is creating new events.

…to event table, update event seeders to generate fake event urls

timmyichen

Do any tests need updating? Do we even have tests for this? 🤔

client/src/modules/dashboard/Events/pages/EventPage.tsx

server/controllers/Events/resolver.ts

gh-23378 · 2020-10-25T21:05:51Z

The only tests I saw were for the mailer

allella · 2020-10-26T14:07:37Z

I'm no good on the technical end, but this does appear to add an event.url as we discussed in #378 and which became #381 so I'm good with it.

@KatieN it sounded like @timmyichen and @Ryuno-Ki were suggesting adding logic to validate the value passed in is actually a valid URL. Could that be added to this PR?

gh-23378 · 2020-10-26T18:41:23Z

Validation is in there :)

server/controllers/Events/resolver.ts

allella

I'm good with this from the business logic. If @timmyichen and @Ryuno-Ki are good then holler and one of us can merge it.

allella · 2020-10-30T14:03:28Z

@Ryuno-Ki if you give the formal approval, then we'll merge this in.

Ryuno-Ki · 2020-10-31T16:36:20Z

Sorry, I was busy the last days. Lemme take a look!

Ryuno-Ki

I'm worried about the lack of validation on URL input.
For example, this could lead to a persistent XSS via javascript: protocol.
But this could be also adressed in a followup-PR.

So I'm approving with comments.

Ryuno-Ki · 2020-10-31T16:37:26Z

client/src/generated/index.tsx

@@ -302,7 +305,7 @@ export type EventsQuery = { __typename?: 'Query' } & {
 events: Array<
 { __typename?: 'Event' } & Pick<
 Event,
- 'id' | 'name' | 'canceled' | 'description' | 'capacity'
+ 'id' | 'name' | 'canceled' | 'description' | 'url' | 'capacity'


Would it make sense to have these ordered alphabetically?
(The OCD in me. Would not block a merge IMHO).

Ryuno-Ki · 2020-10-31T16:37:48Z

client/src/generated/index.tsx

@@ -320,6 +323,7 @@ export type EventQuery = { __typename?: 'Query' } & {
 | 'id'
 | 'name'
 | 'description'
+ | 'url'


Would it make sense to sort these alphabetically?
(Not blocking a merge IMHO).

Ryuno-Ki · 2020-10-31T16:40:23Z

client/src/modules/dashboard/Events/components/EventItem.tsx

@@ -37,6 +37,7 @@ const EventItem: React.FC<IEventItemProps> = ({ loading, event }) => {
 <Typography variant="body2" color="textSecondary" component="p">
 {event.description}
 </Typography>
+ {event.url && <a href={event.url}>{event.url}</a>}


Perhaps worth to wrap in a p, too?

Ryuno-Ki · 2020-10-31T16:40:47Z

client/src/modules/dashboard/Events/pages/EventPage.tsx

@@ -48,6 +48,7 @@ export const EventPage: NextPage = () => {
 <Typography variant="body2" color="textSecondary" component="p">
 {data.event.description}
 </Typography>
+ <a href={data.event.url}>{data.event.url}</a>


Perhaps worth to wrap in a <p>, too?

gh-23378 · 2020-10-31T19:52:13Z

@Ryuno-Ki agreed on sanitization being important, @timmyichen and I were going back and forth on how to handle this here since the suggested new URL() wasn't doing what we wanted (new URL("javascript:alert('foo')") would pass, for example). FYI URL validation has also been removed from my other PR with the assumption it would be handled in a later issue

Ryuno-Ki · 2020-10-31T21:21:09Z

Could we at least have an issue for that?
It would be handy if it would link to those discussions here and your other PR.

allella · 2020-11-01T18:20:33Z

We can continue the conversation on URL validation on #430. If there are no final points, then we'll merge #423 and #424.

allella · 2020-11-01T18:21:48Z

@Ryuno-Ki agreed to proceed on Discord

Looked at #423. Fine with me, but make sure to fill some issues / followup-PRs, @allella
Yes, sanitizing is important. Input validation as well. Those are the two cornerstones of web security.

gh-23378 added 3 commits October 24, 2020 16:15

add nullable url column to model, create migration to add url column …

7aef5fa

…to event table, update event seeders to generate fake event urls

update UI to display event url

9fb800c

conditionally render anchor if event has a url, add missing type info

a688f86

gh-23378 changed the title ~~Feat/event url~~ feat(client, db): event url Oct 25, 2020

timmyichen reviewed Oct 25, 2020

View reviewed changes

client/src/modules/dashboard/Events/pages/EventPage.tsx Show resolved Hide resolved

server/controllers/Events/resolver.ts Show resolved Hide resolved

gh-23378 added 2 commits October 25, 2020 15:01

event url validation

b7a2709

handle empty strings event urls

a1f1d30

timmyichen reviewed Oct 26, 2020

View reviewed changes

server/controllers/Events/resolver.ts Outdated Show resolved Hide resolved

server/controllers/Events/resolver.ts Outdated Show resolved Hide resolved

remove URL validation

5b2c4b7

gh-23378 mentioned this pull request Oct 26, 2020

Feat(client, api, db): video url #424

Merged

3 tasks

allella self-requested a review October 29, 2020 21:13

allella approved these changes Oct 29, 2020

View reviewed changes

timmyichen approved these changes Oct 30, 2020

View reviewed changes

Ryuno-Ki approved these changes Oct 31, 2020

View reviewed changes

allella mentioned this pull request Nov 1, 2020

Validate URL Fields like events.event_url and events.video_url #430

Closed

allella merged commit ff65c21 into freeCodeCamp:master Nov 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(client, db): event url #423

feat(client, db): event url #423

gh-23378 commented Oct 25, 2020

timmyichen left a comment

gh-23378 commented Oct 25, 2020

allella commented Oct 26, 2020

gh-23378 commented Oct 26, 2020

allella left a comment

allella commented Oct 30, 2020

Ryuno-Ki commented Oct 31, 2020

Ryuno-Ki left a comment

Ryuno-Ki Oct 31, 2020

Ryuno-Ki Oct 31, 2020

Ryuno-Ki Oct 31, 2020

Ryuno-Ki Oct 31, 2020

gh-23378 commented Oct 31, 2020

Ryuno-Ki commented Oct 31, 2020

allella commented Nov 1, 2020

allella commented Nov 1, 2020

feat(client, db): event url #423

feat(client, db): event url #423

Conversation

gh-23378 commented Oct 25, 2020

timmyichen left a comment

Choose a reason for hiding this comment

gh-23378 commented Oct 25, 2020

allella commented Oct 26, 2020

gh-23378 commented Oct 26, 2020

allella left a comment

Choose a reason for hiding this comment

allella commented Oct 30, 2020

Ryuno-Ki commented Oct 31, 2020

Ryuno-Ki left a comment

Choose a reason for hiding this comment

Ryuno-Ki Oct 31, 2020

Choose a reason for hiding this comment

Ryuno-Ki Oct 31, 2020

Choose a reason for hiding this comment

Ryuno-Ki Oct 31, 2020

Choose a reason for hiding this comment

Ryuno-Ki Oct 31, 2020

Choose a reason for hiding this comment

gh-23378 commented Oct 31, 2020

Ryuno-Ki commented Oct 31, 2020

allella commented Nov 1, 2020

allella commented Nov 1, 2020