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 "delete a commons" functionality in commonsController.java #44

Merged
merged 6 commits into from
Mar 12, 2022

Conversation

rsdharmaji
Copy link
Contributor

Overview

the delete method is added to the commons controller

Issues Addressed

closes #32

Details

delete method supports an id for deletion, and throws an EntityNotFoundException if deleting nonexistent commons.
ADMIN authentication is required to use the delete method.

here is an "after" image:
image

@codecov
Copy link

codecov bot commented Mar 9, 2022

Codecov Report

Merging #44 (ea552fd) into main (fbf08f8) will increase coverage by 0.51%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #44      +/-   ##
============================================
+ Coverage     78.78%   79.30%   +0.51%     
- Complexity       59       61       +2     
============================================
  Files            51       51              
  Lines           396      401       +5     
  Branches         10       10              
============================================
+ Hits            312      318       +6     
+ Misses           84       83       -1     
Impacted Files Coverage Δ
...156/happiercows/controllers/CommonsController.java 100.00% <100.00%> (ø)
...b/cs156/happiercows/controllers/ApiController.java 100.00% <0.00%> (+14.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbf08f8...ea552fd. Read the comment docs.

jacksoncooper
jacksoncooper previously approved these changes Mar 9, 2022
Copy link
Contributor

@jacksoncooper jacksoncooper left a comment

Choose a reason for hiding this comment

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

Use proper HTTP statuses when the server replies later.

@rsdharmaji rsdharmaji requested a review from kheff16 March 9, 2022 03:57
Jacqueskim
Jacqueskim previously approved these changes Mar 9, 2022
Copy link
Contributor

@Jacqueskim Jacqueskim left a comment

Choose a reason for hiding this comment

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

LGTM

@kheff16
Copy link
Contributor

kheff16 commented Mar 10, 2022

Needs rebase or merge with main but otherwise lgtm

@jacksoncooper jacksoncooper self-requested a review March 11, 2022 06:39
jacksoncooper
jacksoncooper previously approved these changes Mar 11, 2022
Copy link
Contributor

@jacksoncooper jacksoncooper left a comment

Choose a reason for hiding this comment

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

I don't think we broke anything merging.

@jacksoncooper
Copy link
Contributor

#52 affects this.

Copy link
Contributor

@jacksoncooper jacksoncooper left a comment

Choose a reason for hiding this comment

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

This patch doesn't work, unfortunately.

@pconrad pconrad temporarily deployed to team04-w22-6pm-4-qa March 12, 2022 05:26 Inactive
@pconrad pconrad temporarily deployed to team04-w22-6pm-4-qa March 12, 2022 05:38 Inactive
@pconrad pconrad added the 10 label Mar 12, 2022
@pconrad pconrad merged commit 661f4a1 into main Mar 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6pm-4: Add DELETE method to the commons controller.
6 participants