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 post request for comment creation #60

Merged

Conversation

hiin3d55
Copy link
Contributor

@hiin3d55 hiin3d55 commented Mar 18, 2022

Description

Implement HTTP POST request for creating new comments for a forum post.

Completed by both @hiin3d55 and @R055A .

Related Issue

N/A

Solves

Implement post request for creating a comment on a forum post #17

Type of change

  • New feature (enhancement)

How Has This Been Tested?

  • Automated testing
  • Manual testing

Checklist:

  • 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?
  • Have dependent changes been merged and published in downstream modules?
  • 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?

For more information, refer to the Contributing Guidelines and Code of Conduct links at the bottom of this page.

models/comment.server.model.js Outdated Show resolved Hide resolved
controllers/comment.server.controller.js Outdated Show resolved Hide resolved
test/schemas.test.js Outdated Show resolved Hide resolved
@R055A R055A self-assigned this Mar 19, 2022
@R055A R055A self-requested a review March 19, 2022 00:58
@R055A R055A added the enhancement New feature or request label Mar 19, 2022
@R055A R055A added this to the Assignment 1 milestone Mar 19, 2022
@hiin3d55 hiin3d55 marked this pull request as ready for review March 19, 2022 01:01
@hiin3d55 hiin3d55 requested a review from a team as a code owner March 19, 2022 01:01
@R055A R055A linked an issue Mar 19, 2022 that may be closed by this pull request
@hiin3d55 hiin3d55 assigned hiin3d55 and unassigned R055A Mar 19, 2022
Copy link
Contributor

@R055A R055A left a comment

Choose a reason for hiding this comment

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

I have left comments with suggestion where I think changes are required.

There is already an existing route for posting a new comment to an existing forum post:

    app.route('/api/v1/posts/:id/comments')
        .get(forum.commentViewById)
        .post(forum.commentGiveById);

    app.route('/api/v1/posts/:id/comments/:id')
        .patch(forum.commentUpdateById);

This can be found in: routes/forum.server.routes.js

There are dummy functions with TODO comments already existing in the forum.controller for implementing forum post comment features.

In terms of cohesion I think it would be better to keep all the comment functionality in the forum controller and model files. Keep in mind making new comment files would create more coupling than just between forum post and forum comment. A forum has posts and posts have comments. A forum user makes forum posts and comments to forum posts. If we remove forum posts (hypothetically), comments could still remain but without posts to comment to.

config/express.server.config.js Outdated Show resolved Hide resolved
config/db_schemas/forum.schema.js Outdated Show resolved Hide resolved
config/db_schemas/comment.schema.js Outdated Show resolved Hide resolved
models/db.server.model.js Show resolved Hide resolved
models/db.server.model.js Show resolved Hide resolved
routes/comment.server.routes.js Outdated Show resolved Hide resolved
test/schemas.test.js Outdated Show resolved Hide resolved
test/comment.test.js Outdated Show resolved Hide resolved
test/comment.test.js Outdated Show resolved Hide resolved
test/comment.test.js Outdated Show resolved Hide resolved
@R055A R055A assigned R055A and unassigned hiin3d55 Mar 19, 2022
@R055A
Copy link
Contributor

R055A commented Mar 19, 2022

I have reassigned myself to this pull request because I am responsible for merging the pull request when it is approved

config/db_schemas/forum.schema.js Outdated Show resolved Hide resolved
controllers/comment.server.controller.js Outdated Show resolved Hide resolved
controllers/comment.server.controller.js Outdated Show resolved Hide resolved
@R055A R055A self-requested a review March 19, 2022 05:42
Copy link
Contributor

@R055A R055A left a comment

Choose a reason for hiding this comment

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

Aside for the requested changes it looks very good!

The PR includes changes that are not necessarily required for this feature.

For this feature, no new files should be required in the following directories:
routes/
controllers/
models/

For this feature, no changes should be required in the following files:
models/db.server.model.js
config/express.server.config.js

It's better to have high cohesion and low coupling. Making new files for comments increases coupling and reduces cohesion.

config/db_schemas/comment.schema.js Outdated Show resolved Hide resolved
config/db_schemas/comment.schema.js Show resolved Hide resolved
config/db_schemas/comment.schema.js Outdated Show resolved Hide resolved
config/db_schemas/comment.schema.js Show resolved Hide resolved
config/db_schemas/forum.schema.js Outdated Show resolved Hide resolved
controllers/comment.server.controller.js Outdated Show resolved Hide resolved
controllers/comment.server.controller.js Outdated Show resolved Hide resolved
controllers/comment.server.controller.js Outdated Show resolved Hide resolved
controllers/comment.server.controller.js Outdated Show resolved Hide resolved
models/comment.server.model.js Outdated Show resolved Hide resolved
@R055A R055A requested review from R055A and kimslor March 20, 2022 19:39
@R055A R055A merged commit bb8427a into SE701-T5:main Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement post request for creating a comment on a forum post
3 participants