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

Database #25

Merged
merged 24 commits into from Jun 1, 2022
Merged

Database #25

merged 24 commits into from Jun 1, 2022

Conversation

FranziGraml
Copy link
Owner

@FranziGraml FranziGraml commented May 31, 2022

In this PR I connected my database (MongoDB Atlas) with Add and Delete Button.
Sorry for all the commits at the end but my github checks kept failing! Sorry!!!

@vercel
Copy link

vercel bot commented May 31, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
capstone-project ✅ Ready (Inspect) Visit Preview Jun 1, 2022 at 1:46PM (UTC)

Copy link

@F-Kirchhoff F-Kirchhoff left a comment

Choose a reason for hiding this comment

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

Hey Franzi, looking good so far. I have only some minor suggestions about error handling and state management. Besides that its top!

Comment on lines 7 to 8
const deletedPost = await Post.findByIdAndDelete(id);
res.status(200).json({ message: 'post deleted', post: deletedPost });

Choose a reason for hiding this comment

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

what happens if the deletion fails? I guess you can add a try and catch block here.

Comment on lines +9 to +10
} else {
const singleCard = await Post.findById(id);

Choose a reason for hiding this comment

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

Even when a 'POST' or 'PUT' request is made this code is executed. Please add another else if block to check whether the method is 'GET'. (You could use a switch statement as well).


response.status(200).json({ message: 'post created', post: newPost });
} else {
response.status(400).json({ error: 'wrong method' });

Choose a reason for hiding this comment

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

the correct status code is 405.

Copy link

Choose a reason for hiding this comment

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

What Felix said

Comment on lines 15 to 23
const newPost = await Post.create({
name: data.name,
content: data.post,
mail: data.mail,
mobile: data.mobile,
postDate: data.postDate,
});

response.status(200).json({ message: 'post created', post: newPost });

Choose a reason for hiding this comment

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

Here you could also add error handling if you like.

Copy link

Choose a reason for hiding this comment

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

Error-handling still missing :P

pages/posts.js Outdated
const data = getPosts();
export async function getStaticProps() {
const posts = await getPosts();
console.log('posts: ' + JSON.stringify(posts));

Choose a reason for hiding this comment

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

Please remove all logs before merging 🕵️

Copy link

Choose a reason for hiding this comment

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

Still there? ;)

Comment on lines 14 to 17
const [nameValue, setNameValue] = useState('');
const [postValue, setPostValue] = useState('');
const [mailValue, setMailValue] = useState('');
const [mobileValue, setMobileValue] = useState('');

Choose a reason for hiding this comment

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

Maybe you can refactor this into one object state at some point?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I will do this in one of my next user storys. :)

setNameValue('');
setPostValue('');
setMailValue('');
setMobileValue('');
onSetIsFormActive(false);
setIsError(false);
console.log(await response.json);

Choose a reason for hiding this comment

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

🕵️

Copy link
Owner Author

Choose a reason for hiding this comment

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

same here

Copy link

Choose a reason for hiding this comment

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

You did not remove that?

{posts
.sort((a, b) => b.postDate - a.postDate)
.map(post => {
return (

Choose a reason for hiding this comment

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

you could use an implicit return here:

post => (
  <ul key = ...>
  ...
  </ul>
)

dotenv.config({ path: '../../../.env.local' });

const url = process.env.DB_CONNECTION; /* 'mongodb://localhost:27017/digi-nomads'; */
console.log(url);

Choose a reason for hiding this comment

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

🕵️

Comment on lines 1 to 20
[
{
"name": "Franzi",
"content": "Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. ",
"mail": "hallo.welt@com",
"mobile": "2367654"
},
{
"name": "Max",
"content": "Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. ",
"mail": "hallo.world@com",
"mobile": "25598078"
},
{
"name": "Muc-web",
"content": "Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. ",
"mail": "hallo.terra@com",
"mobile": "648893678"
}
]

Choose a reason for hiding this comment

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

You don't need this any longer if you have a db right?

@FranziGraml
Copy link
Owner Author

Thanks a lot for the nice and helpful feedback!

Copy link

@doemser doemser left a comment

Choose a reason for hiding this comment

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

You replied with "thumbs up" to most of the changes Felix requested, but did not implement them.

.sort((a, b) => b.postDate - a.postDate)
.map(post => {
return (
<ul key={nanoid()}>
Copy link

@doemser doemser Jun 1, 2022

Choose a reason for hiding this comment

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

take your post.id as key instead

setNameValue('');
setPostValue('');
setMailValue('');
setMobileValue('');
onSetIsFormActive(false);
setIsError(false);
console.log(await response.json);
Copy link

Choose a reason for hiding this comment

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

You did not remove that?

pages/posts.js Outdated
Comment on lines 34 to 36
<FormWrapper isFormActive={isFormActive}>
<Form onSetIsFormActive={setIsFormActive} />
</FormWrapper>
Copy link

Choose a reason for hiding this comment

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

Here you pass down the setIsFormActive function directly into your Form. This is something you should avoid.

Instead do something like this:

function deactivateForm() {
setIsFormActive(!isFormActive);
}

An then pass it down like this:

<Form onSetIsFormActive={deactivateForm} />

This way you don't separate the logic from the useState, means: You can see which changes to the useState can be applied without leaving the place where the useState was written.

This procedure is very important and is called "Lifting state up". Here is a useful Link

pages/posts.js Outdated
const data = getPosts();
export async function getStaticProps() {
const posts = await getPosts();
console.log('posts: ' + JSON.stringify(posts));
Copy link

Choose a reason for hiding this comment

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

Still there? ;)


response.status(200).json({ message: 'post created', post: newPost });
} else {
response.status(400).json({ error: 'wrong method' });
Copy link

Choose a reason for hiding this comment

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

What Felix said

Comment on lines 15 to 23
const newPost = await Post.create({
name: data.name,
content: data.post,
mail: data.mail,
mobile: data.mobile,
postDate: data.postDate,
});

response.status(200).json({ message: 'post created', post: newPost });
Copy link

Choose a reason for hiding this comment

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

Error-handling still missing :P

@FranziGraml
Copy link
Owner Author

Hi Dominik
Thanks for you reviewing. I hope that this time my changes were loaded correctly.

Copy link

@F-Kirchhoff F-Kirchhoff left a comment

Choose a reason for hiding this comment

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

Very cool 🦊. There are still some little tweaks you need to add, but you are almost done!

const deletedPost = await Post.findByIdAndDelete(id);
res.status(200).json({ message: 'post deleted', post: deletedPost });
} catch (error) {
console.log('Error could not delete', error.message);

Choose a reason for hiding this comment

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

If you end up in this catch block, the api does not send a response to your client. This will result in a timeout. You should send a status 501 for internal server error back to the client.

response.status(200).json({ message: 'post created', post: newPost });
response.status(200).json({ message: 'post created', post: newPost });
} catch (error) {
console.log('Error could not POST', error.message);

Choose a reason for hiding this comment

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

same here

Comment on lines +26 to +28
function setFormActiveFalse() {
setIsFormActive(false);
}

Choose a reason for hiding this comment

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

nice!

return (
<main>
<SWRConfig value={{ fetcher: swrFetcher, fallback }}>
<ButtonAdd type="button" onClick={() => setIsFormActive(prevCheck => !prevCheck)}>
<Icon variant="plus" />
</ButtonAdd>
<FormWrapper isFormActive={isFormActive}>
<Form onSetIsFormActive={setIsFormActive} />
<Form onSetFormActiveFalse={() => setFormActiveFalse()} />

Choose a reason for hiding this comment

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

Suggested change
<Form onSetFormActiveFalse={() => setFormActiveFalse()} />
<Form onSetFormActiveFalse={setFormActiveFalse} />

This works as well and is a bit shorter. But your solution is nice too.

@@ -11,10 +11,9 @@ export default function DeleteButton({ id }) {
<ButtonDelete
type="button"
onClick={async () => {
const response = await fetch('/api/post/' + id, {
const _response = await fetch('/api/post/' + id, {

Choose a reason for hiding this comment

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

if you don't want to use the response you can just omit the constant declaration:

Suggested change
const _response = await fetch('/api/post/' + id, {
await fetch('/api/post/' + id, {

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good to know. I would still like to keep it as I was very pleased with my solution.

@@ -1,6 +1,6 @@
import PostCard from '../PostCard/PostCard';
import useSWR from 'swr';
import { nanoid } from 'nanoid';
//import { nanoid } from 'nanoid';

Choose a reason for hiding this comment

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

Please remove code instead of commenting it out ;)

Copy link

@F-Kirchhoff F-Kirchhoff left a comment

Choose a reason for hiding this comment

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

This looks really good now! 🚀

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.

None yet

3 participants