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

#167966138 Enable user to update and delete articles #27

Merged
merged 3 commits into from
Aug 23, 2019

Conversation

Jkadhuwa
Copy link
Contributor

@Jkadhuwa Jkadhuwa commented Aug 21, 2019

What does this PR do?

Enables user to update, delete and get single article

How should this be manually tested?

  • git fetch

  • git checkout ft-user-update-delete-articles-167966138

  • npm install

  • npm run dev

  • Using postman test using the following endpoints

     put /api/v1/articles/:slug
       - and supply the fields you want to update
    
    delete /api/v1/articles/:slug 
       - to delete specified article
    
    get /api/v1/articles/:slug 
       - to get the specified article
    
    

What are the relevant pivotal tracker stories?

Update or Delete Articles

Screenshots

Screen Shot 2019-08-21 at 17 05 32

Screen Shot 2019-08-21 at 17 04 07

Screen Shot 2019-08-23 at 10 20 32

@@ -0,0 +1,22 @@
import bcrypt from 'bcrypt';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,24 @@
import { Article } from '../sequelize/models';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@Jkadhuwa Jkadhuwa force-pushed the ft-user-update-and-delete-articles-167966138 branch from e144bba to 429b0ca Compare August 21, 2019 17:59
swagger.json Outdated
}
}
},
"404": {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the responses status code, I don't see 403 whereby the user is not allowed to delete an article that does not belong to them for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

static async deteleArticle(req, res) {
try {
const deleted = await Article.destroy({
where: { slug: req.userData.slug }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you use slug instead of ID to delete article?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already had slug from the params, I thought it would be efficient to delete using it rather than going to the DB search for an article using slug so as to get the id. But I have got another way of getting the id which I have implemented it.

}
article.description = (description || originalArticle.description).trim();
article.body = (body || originalArticle.body).trim();
article.tagList = (tags || originalArticle.tagList.toString()).trim().split(/[ ,]+/);
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 update an article that didn't have previous tags throws TypeError: Cannot read property 'toString' of null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I found that also and fixed it. I will be pushing the changes in a min.

article.description = (description || originalArticle.description).trim();
article.body = (body || originalArticle.body).trim();
article.tagList = (tags || originalArticle.tagList.toString()).trim().split(/[ ,]+/);
article.category = (category || originalArticle.category).trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

why trim originalArticle.category?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I am checking for the category from req.body to trim but when I define (category.trim() || originalArticle.category) throws an error when the category is undefined

article.title = title.trim();
article.slug = slugGen(title.trim());
}
article.description = (description || originalArticle.description).trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

article.description = description.trim() || originalArticle.description;

Copy link
Contributor

Choose a reason for hiding this comment

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

I also see that trim has been added to the validation rules, is there a reason we have it here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the description is undefined it throws an error "can not trim value of undefined"

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 also see that trim has been added to the validation rules, is there a reason we have it here as well?

In the validations am checking for empty fields but in the controller am trimming white spaces before or after a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

article.description = description ? description.trim() : originalArticle.description;

);
if (updatedArticle) {
return res.status(200).json({
message: 'Profile updated successfully',
Copy link
Contributor

Choose a reason for hiding this comment

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

Article updated successfully

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 am changing that.

article.title = title.trim();
article.slug = slugGen(title.trim());
}
article.description = (description || originalArticle.description).trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

I also see that trim has been added to the validation rules, is there a reason we have it here as well?

@Jkadhuwa Jkadhuwa force-pushed the ft-user-update-and-delete-articles-167966138 branch from d9c177e to ffe884d Compare August 22, 2019 18:31
@Jkadhuwa Jkadhuwa force-pushed the ft-user-update-and-delete-articles-167966138 branch from ffe884d to 8fda042 Compare August 22, 2019 18:42
article.title = title.trim();
article.slug = slugGen(title.trim());
}
article.description = (description || originalArticle.description).trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

article.description = description ? description.trim() : originalArticle.description;

@Jkadhuwa Jkadhuwa force-pushed the ft-user-update-and-delete-articles-167966138 branch from 8fda042 to 0d7d7d9 Compare August 22, 2019 19:37
@raymond42 raymond42 temporarily deployed to codinggeeks-ah-backnd-st-pr-27 August 23, 2019 08:18 Inactive
@raymond42 raymond42 added finished and removed Need Reviews Feedback to PR raised labels Aug 23, 2019
@Jkadhuwa Jkadhuwa force-pushed the ft-user-update-and-delete-articles-167966138 branch from 035cf4a to 1c657fb Compare August 23, 2019 11:09
@raymond42 raymond42 added Need Reviews Feedback to PR raised and removed finished labels Aug 23, 2019
@mkiterian mkiterian merged commit 3e22c8f into development Aug 23, 2019
@Jkadhuwa Jkadhuwa removed the Need Reviews Feedback to PR raised label Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants