w22-6pm-3 Profits #43

eambriz27 · 2022-03-09T02:53:56Z

Overview

In this PR, we implemented the entity, repository, and controller for the Profits. We also implemented a test suite for all backend components to reach full code coverage for all new code written. We also implemented all the frontend components necessary for our backend components to be visible in the profits table.

Issues Addressed

This issue addresses issue #21.

Details

Backend Development:
Our goal during developing the backend for this issue was to create a Profits Controller that supported CRUD operations. This included creating an entity, repository, and controller for the Profits component.

The API endpoints created in the Profits Controller are as follows:
Get /api/profits: Gets a single profit for a user
Get /api/profits/admin: Gets a single profit as admin
Get /api/profits/admin/all: lists all profits
Get /api/profits/admin/all/commons: gets all profits belonging to a user as an admin
Get /api/profits/all/commons: Get all profits belonging to a user commons as an admin
Put /api/profits/admin: Updates a single profit as an admin
Delete /api/profits/admin: Deletes other users profit as admin
Post /api/profits/admin: Creates a new profit as admin

These endpoints can also be seen in the swagger-ui.

We then implemented tests for the Profits controller, focusing on writing tests for each CRUD operation, with both admin and user roles.

Above shows 100% jacoco coverage for the Profits controller.

Frontend Development:
The issues addressed in the frontend portion of this PR are as follows:
Created fixtures for Profits and ProfitsTable
Profits table is visible to the user with correct formatting
Profits table shows the correct profit values
Profits table displays formatted dates using profit timestamps
Profits table updates profit values
Profits table works with no profits
Wrote tests for Profits and ProfitsTable functionality

Profits table functionality:

Maven test results:

Jest coverage results:

codecov · 2022-03-09T03:03:34Z

Codecov Report

Merging #43 (fc2d903) into main (661f4a1) will increase coverage by 1.79%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main      #43      +/-   ##
============================================
+ Coverage     79.30%   81.09%   +1.79%     
- Complexity       61       75      +14     
============================================
  Files            51       52       +1     
  Lines           401      439      +38     
  Branches         10       12       +2     
============================================
+ Hits            318      356      +38     
  Misses           83       83

Impacted Files	Coverage Δ
frontend/src/main/components/Commons/Profits.js	`100.00% <100.00%> (ø)`
frontend/src/main/pages/PlayPage.js	`100.00% <100.00%> (ø)`
...156/happiercows/controllers/ProfitsController.java	`100.00% <100.00%> (ø)`

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 661f4a1...fc2d903. Read the comment docs.

…6pm-HappyCows into ea-rm-profits

frontend/src/main/components/Commons/Profits.js

gama-A

Are profits not a variable of user commons? How are the profits in the table here connected to each individual user?

pconrad

So this is a lot of work!

Please, next time, parse this out in smaller pieces! We've been trying to model that throughout the course, separating the front end into little small bits. Separating it from the backend. Doing the backend a little at a time.

Because this is so big, we are going to need a bit more of a description than what you've provided. Normally, this would be 4 or 5 pull requests, and there would be screenshots with each and every one of them.

By combining everything together, it makes is much much harder for us to know how many points to assign. Most other teams get 10 points for this, 10 points for that... one PR at a time.

Please see if you can remove the dependency on moment, and also see if you can edit the description to help us through what you've done here.

frontend/package.json

frontend/src/main/components/Commons/Profits.js

…ent back to add more tests to get to 100%

…6pm-HappyCows into ea-rm-profits

pconrad

I'm really confused by this. Why should individual game players have the ability to generate profits out of thin air?

Surely this seems like it should be an admin only function?

pconrad · 2022-03-12T02:01:34Z

src/main/java/edu/ucsb/cs156/happiercows/controllers/ProfitsController.java

+    @ApiOperation(value = "Update a single profit as user")
+    @PreAuthorize("hasRole('ROLE_USER')")
+    @PutMapping("")
+    public Profit putProfitById(
+            @ApiParam("id") @RequestParam Long id,
+            @RequestBody @Valid Profit newProfit) {
+        Long userId = getCurrentUser().getUser().getId();
+
+        Profit profit = profitRepository.findById(id).orElseThrow(() -> new EntityNotFoundException(Profit.class, id));
+        if (userId != profit.getUserCommons().getUserId())
+            throw new EntityNotFoundException(Profit.class, id);
+
+        profit.setProfit(newProfit.getProfit());
+        profit.setTimestamp(newProfit.getTimestamp());
+
+        profitRepository.save(profit);
+
+        return profit;
+    }
+


What is the use case for a user to update their own profit?

This seems like a way that a user could cheat and just give themselves more money.

pconrad · 2022-03-12T02:02:03Z

src/main/java/edu/ucsb/cs156/happiercows/controllers/ProfitsController.java

+    @ApiOperation(value = "Delete a Profit of the user")
+    @PreAuthorize("hasRole('ROLE_USER')")
+    @DeleteMapping("")
+    public Object deleteProfit(
+            @ApiParam("id") @RequestParam Long id) {
+        Long userId = getCurrentUser().getUser().getId();
+
+        Profit profit = profitRepository.findById(id).orElseThrow(() -> new EntityNotFoundException(Profit.class, id));
+
+        if (userId != profit.getUserCommons().getUserId())
+            throw new EntityNotFoundException(Profit.class, id);
+
+        profitRepository.delete(profit);
+
+        return genericMessage("Profit with id %s deleted".formatted(id));
+    }
+}


What is the user case for a user to delete their own profit?

Shouldnt' this be an admin only function?

pconrad · 2022-03-12T02:02:22Z

src/main/java/edu/ucsb/cs156/happiercows/repositories/ProfitRepository.java

+
+@Repository
+public interface ProfitRepository extends CrudRepository<Profit, Long> {
+    //Iterable<Profit> findAll();


What is this commented out line of code doing here?

Apologies, I meant to take it out after testing as I found the CrudRepository already comes with findAll();, it will take it out.

pconrad · 2022-03-12T02:03:33Z

src/main/java/edu/ucsb/cs156/happiercows/controllers/ProfitsController.java

+    @ApiOperation(value = "Create a new Profit as user")
+    @PreAuthorize("hasRole('ROLE_USER')")
+    @PostMapping("/post")
+    public Profit postProfit(
+            @ApiParam("profit") @RequestParam long profit,
+            @ApiParam("timestamp") @RequestParam long timestamp,
+            @ApiParam("userCommonsId") @RequestParam long userCommonsId) {
+
+        Long userId = getCurrentUser().getUser().getId();
+        UserCommons userCommons = userCommonsRepository.findById(userCommonsId).orElseThrow(() -> new EntityNotFoundException(UserCommons.class, userCommonsId));
+
+        Profit createdProfit = new Profit();
+        createdProfit.setProfit(profit);
+        createdProfit.setUserCommons(userCommons);
+        createdProfit.setTimestamp(timestamp);
+        Profit savedProfit = profitRepository.save(createdProfit);
+        return savedProfit;
+    }


What is the user case for a user to create a profit?

Doesn't that just give users the ability to cheat and give themselves money any time they want?

pconrad

LGTM

eambriz27 and others added 6 commits March 5, 2022 13:05

added repository

edf58b3

added entity and controller

4260c6a

finished controller

d987439

started controller tests

e638b59

rm - working on profits frontend

c05e50d

ea-working on controller tests, at about 25% cov

f81d78b

eambriz27 added the w22-6pm-3 label Mar 9, 2022

eambriz27 requested a review from btk5h March 9, 2022 02:53

eambriz27 assigned btk5h, ryanmosalem and eambriz27 Mar 9, 2022

fixing one failing test

16d31d1

ryanmosalem added 3 commits March 8, 2022 23:11

rm - updating formatting for dates

b7a40c9

Merge branch 'ea-rm-profits' of github.com:ucsb-cs156-w22/team04-w22-…

b6e1385

…6pm-HappyCows into ea-rm-profits

rm - fixing errors and adding tests

a4ea495

eambriz27 marked this pull request as draft March 9, 2022 07:35

eambriz27 requested review from bzamora020 and removed request for btk5h March 9, 2022 08:00

eambriz27 unassigned btk5h Mar 9, 2022

eambriz27 and others added 3 commits March 9, 2022 00:03

Merge branch 'main' into ea-rm-profits

01dd14b

rm - finished profitsTable tests

05bf7c3

Merge branch 'ea-rm-profits' of github.com:ucsb-cs156-w22/team04-w22-…

ea996da

…6pm-HappyCows into ea-rm-profits

bzamora020 requested changes Mar 9, 2022

View reviewed changes

frontend/src/main/components/Commons/Profits.js Outdated Show resolved Hide resolved

bzamora020 added Attn-peer-CR At least one team member should do a code review before the staff Attn-respond-to-CR Please address changes requested in code review labels Mar 9, 2022

rm - deleteing unused code

fc72cf6

gama-A reviewed Mar 9, 2022

View reviewed changes

ryanmosalem and others added 2 commits March 9, 2022 13:02

rm - deleteing unused code

a6fbae4

finished test suite for Profits controller

5bf3fc5

eambriz27 changed the title ~~Profits Backend done with the exception of 100% jacoco coverage~~ Profits Mar 11, 2022

Merge branch 'main' into ea-rm-profits

ff6a334

pconrad requested changes Mar 11, 2022

View reviewed changes

frontend/package.json Outdated Show resolved Hide resolved

frontend/src/main/components/Commons/Profits.js Outdated Show resolved Hide resolved

jacoco error was previously showing 100%, then realized it wasnt so w…

01840e2

…ent back to add more tests to get to 100%

eambriz27 dismissed gama-A’s stale review via 01840e2 March 11, 2022 07:43

eambriz27 temporarily deployed to team04-w22-6pm-3 March 11, 2022 19:13 Inactive

eambriz27 changed the title ~~Profits~~ w22-6pm-3 Profits Mar 11, 2022

trying to fix api endpoint error heroku

579dc64

eambriz27 temporarily deployed to team04-w22-6pm-3 March 11, 2022 19:58 Inactive

eambriz27 temporarily deployed to team04-w22-6pm-3 March 11, 2022 20:12 Inactive

eambriz27 temporarily deployed to team04-w22-6pm-3 March 11, 2022 20:43 Inactive

eambriz27 temporarily deployed to team04-w22-6pm-3 March 11, 2022 21:54 Inactive

eambriz27 temporarily deployed to team04-w22-6pm-3 March 11, 2022 22:02 Inactive

eambriz27 and others added 3 commits March 11, 2022 14:19

removed findAll from profits repository as it comes with CRUD

4beab2b

rm - fixed timestampToDate function to remove moment

22e6839

Merge branch 'ea-rm-profits' of github.com:ucsb-cs156-w22/team04-w22-…

6ef73c0

…6pm-HappyCows into ea-rm-profits

ryanmosalem requested a review from pconrad March 11, 2022 23:29

Merge branch 'main' into ea-rm-profits

343abce

pconrad requested changes Mar 12, 2022

View reviewed changes

removing PUT, DELETE, POST endpoints for users and associated tests

e244588

eambriz27 requested a review from pconrad March 12, 2022 19:18

Merge branch 'main' into ea-rm-profits

fc2d903

pconrad approved these changes Mar 12, 2022

View reviewed changes

pconrad temporarily deployed to team04-w22-6pm-3-qa March 12, 2022 19:25 Inactive

pconrad temporarily deployed to team04-w22-6pm-3-qa March 12, 2022 20:30 Inactive

pconrad added 20 10 and removed Attn-respond-to-CR Please address changes requested in code review Attn-peer-CR At least one team member should do a code review before the staff labels Mar 12, 2022

pconrad merged commit 0b7a9ba into main Mar 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

w22-6pm-3 Profits #43

w22-6pm-3 Profits #43

eambriz27 commented Mar 9, 2022 •

edited

codecov bot commented Mar 9, 2022 •

edited

gama-A left a comment

pconrad left a comment

pconrad left a comment

pconrad Mar 12, 2022

pconrad Mar 12, 2022

pconrad Mar 12, 2022

eambriz27 Mar 12, 2022

pconrad Mar 12, 2022

pconrad left a comment

w22-6pm-3 Profits #43

w22-6pm-3 Profits #43

Conversation

eambriz27 commented Mar 9, 2022 • edited

Overview

Issues Addressed

Details

codecov bot commented Mar 9, 2022 • edited

Codecov Report

gama-A left a comment

Choose a reason for hiding this comment

pconrad left a comment

Choose a reason for hiding this comment

pconrad left a comment

Choose a reason for hiding this comment

pconrad Mar 12, 2022

Choose a reason for hiding this comment

pconrad Mar 12, 2022

Choose a reason for hiding this comment

pconrad Mar 12, 2022

Choose a reason for hiding this comment

eambriz27 Mar 12, 2022

Choose a reason for hiding this comment

pconrad Mar 12, 2022

Choose a reason for hiding this comment

pconrad left a comment

Choose a reason for hiding this comment

eambriz27 commented Mar 9, 2022 •

edited

codecov bot commented Mar 9, 2022 •

edited