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

Implement Forum Post Update Feature using HTTP PATCH #53

Merged
merged 3 commits into from
Mar 18, 2022
Merged

Implement Forum Post Update Feature using HTTP PATCH #53

merged 3 commits into from
Mar 18, 2022

Conversation

R055A
Copy link
Contributor

@R055A R055A commented Mar 18, 2022

Description

Implement update feature that responds to HTTP PATCH requests for updates of any valid editable forum post document field(s) with the updated forum post data.

Refactored existing forum controller functions to use added validation functionality.

Related Issue

Solves #27

Type of change

  • New feature (enhancement)

How Has This Been Tested?

Multiple automated test cases for

  • successfully updating all editable forum post document fields
  • successfully updating only up/down vote fields
  • successfully updating only editable fields
  • unsuccessfully updating with invalid forum post ID
  • unsuccessfully updating without any fields
  • unsuccessfully updating with invalid fields
  • Automated testing

Checklist:

A similar pull request already exists for this, but it is made by the same GitHub user. The reason it is duplicated is to fix an issue with commits from prior PRs that aren't yet merged being added to new PRs which is confusing reviewers. To fix the issue, the repository fork the initial PR was made from was deleted. When that fork reposotiry was deleted the initial PR automatically closed and cannot be merged because of this. In the past when I delete a fork repository the PRs from that repository have not been closed, but in this case the PR was closed. A new fork was made to the same GitHub account. The same changes have been pushed to the new fork repository and a duplicate PR has been required for merging those changes. The now closed initial PR is #50

  • Does a similar (open or closed) pull request not already exist?
  • Is the pull request head repository a fork repository?
  • Is the pull request compare branch a development branch?
  • Is the code documented, particularly in hard-to-understand areas?
  • Does the code build without new warnings?
  • Has testing been performed that proves changes are effective and work?
  • Has a self- and/or peer-review of the code been performed?
  • Does all new and existing automated testing pass?
  • Is the person responsible for the repository assigned to the pull request?
  • Is the pull request linked to a project?
  • Is the pull request linked to a milestone?

refactor forum controller functions to use added validate func
add test cases for forum post updates
@R055A R055A added the enhancement New feature or request label Mar 18, 2022
@R055A R055A added this to the Assignment 1 milestone Mar 18, 2022
@R055A R055A requested a review from a team as a code owner March 18, 2022 02:37
@R055A R055A self-assigned this Mar 18, 2022
@R055A R055A linked an issue Mar 18, 2022 that may be closed by this pull request
Copy link
Contributor

@hiin3d55 hiin3d55 left a comment

Choose a reason for hiding this comment

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

Big work! But a lot going on in this PR. Wouldn't it be better to split up the work amongst multiple PRs? Also, looks like there was only one big commit instead of multiple small commits. This is just what I'm thinking so let me know if there were any particular reasons for implementing it in this way.

@R055A
Copy link
Contributor Author

R055A commented Mar 18, 2022

Big work! But a lot going on in this PR. Wouldn't it be better to split up the work amongst multiple PRs? Also, looks like there was only one big commit instead of multiple small commits. This is just what I'm thinking so let me know if there were any particular reasons for implementing it in this way.

I implemented the feature described in the issue. I added test cases for this. I did some reafctoring to related existing features that are already merged and no longer being worked on my anyone. I refactored to reduce code reuse by adding validation functions and added calls to these functions in place of where code was being repeated.

Regarding the changes to user.controller, user.model and user.test, I am unaware why they have been included. I will remove them. Thank you for bringing this to my attention.

@R055A
Copy link
Contributor Author

R055A commented Mar 18, 2022

Big work! But a lot going on in this PR. Wouldn't it be better to split up the work amongst multiple PRs? Also, looks like there was only one big commit instead of multiple small commits. This is just what I'm thinking so let me know if there were any particular reasons for implementing it in this way.

I implemented the feature described in the issue. I added test cases for this. I did some reafctoring to related existing features that are already merged and no longer being worked on my anyone. I refactored to reduce code reuse by adding validation functions and added calls to these functions in place of where code was being repeated.

Regarding the changes to user.controller, user.model and user.test, I am unaware why they have been included. I will remove them. Thank you for bringing this to my attention.

The unwanted changes are now removed. The only changes that remain in any file relating to the user features are the addition of empty lines at the bottom of a file, the removal of an unwanted empty line at the top of a file and the removal of unwanted empty spaces at the beginning of functions declarations.

Copy link
Contributor

@hiin3d55 hiin3d55 left a comment

Choose a reason for hiding this comment

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

Made a comment previously due to large commits and a lot of content for the PR. However, this was due to a fork-related issue. For future PRs, it will be better to make sure the commits are frequent and small and the PR doesn't contain wide variety of changes.

Otherwise, looks good to me!

@R055A R055A merged commit e8f1ff6 into SE701-T5:main Mar 18, 2022
@R055A R055A deleted the update-forum-post branch March 18, 2022 06:42
@R055A R055A added the duplicate This issue or pull request already exists label Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement request for editing a forum post
2 participants