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.

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

London 10 -- Kristina Dudnyk -- Fullstack Project Assignment -- DataBases -- 300 #380

Closed
wants to merge 95 commits into from

Conversation

KristinaDudnyk
Copy link

No description provided.

Copy link

@GergelyKI GergelyKI left a comment

Choose a reason for hiding this comment

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

Looks good all together, I left one comment.

Copy link
Member

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

This is generally looking pretty good, but it'd be worth thinking/talking about how to handle related pieces of state (e.g. your ratings state array). Also, did you get on to the database parts yet?

client/src/NewVideo.js Outdated Show resolved Hide resolved
client/src/VideosPerent.js Outdated Show resolved Hide resolved
client/src/SetRating.js Outdated Show resolved Hide resolved
server/server.js Outdated Show resolved Hide resolved
server/server.js Outdated Show resolved Hide resolved
@KristinaDudnyk KristinaDudnyk changed the title London 10 -- Kristina Dudnyk -- Fullstack Project Assignment -- DataBases London 10 -- Kristina Dudnyk -- Fullstack Project Assignment -- DataBases -- 300 Sep 20, 2023
Copy link
Member

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Really nice clean-ups, good job! 🎉

Comment on lines +21 to +23
url: `https://www.youtube-nocookie.com/embed/${
formData.url.match(/v=([a-zA-Z0-9_-]{11})/)[1]
}`,
Copy link
Member

Choose a reason for hiding this comment

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

Can you talk through what this code is doing?

What assumptions are you making, and how confident are you in them? What could go wrong with it if your assumptions are violated?

Copy link
Author

Choose a reason for hiding this comment

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

It's a regex that returns the first match, which is eleven characters after v= sign
There was a thought that link could have params and queries, depending on user's action.
So that code is preventing this possibility from the very beginning.

const handleUpdate = async () => {
try {
const response = await fetch(
`https://kristinadudnyk-fullstack-project.onrender.com/video/${video.id}`,
Copy link
Member

Choose a reason for hiding this comment

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

I've noticed you've put in this URL a few times, and that in each of them you have a localhost:4500 version commented out.

Can you think of a way to make it easier for you to change which server your frontend is talking to for when you want to test things locally?

Copy link
Author

Choose a reason for hiding this comment

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

A brainy mate suggested that I can use

ssl: {
    rejectUnauthorized: false,
  },

which makes it run locally

app.post("/video", async (req, res) => {
const { title, url } = req.body;
// console.log("post video req.body", req.body);
const id = url.split("embed/")[1];
Copy link
Member

Choose a reason for hiding this comment

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

What assumptions are you making about input from users here? What would happen if those violations were violated? How could they be?

Copy link
Author

Choose a reason for hiding this comment

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

Regex changes it before the request is sent, so it prevents that issue on the front-end side, passing the clean URL to the server.

@Dedekind561 Dedekind561 closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants