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

RL-SM: Add Functionality for onBuy, onSell Buttons, updating totalWealth using 2 PUT endpoints #51

Closed
wants to merge 21 commits into from

Conversation

Lin2
Copy link
Contributor

@Lin2 Lin2 commented Mar 11, 2022

Overview

We added two PUT endpoints for BUY and SELL, and created onBuy and onSell basic functionality to update totalWealth of a user on the Playpage.

Issues Addressed

We addressed some of the points in #15 . This issue isn't completed yet however, but we wanted to get a PR in for CI/CD.

We addressed these issues:

  • PlayPage.js updates TotalWealth (located in backend UserCommons.java) in onBuy and onSell based on Market Cow Price.
  • Get FarmStats.js to display Total Wealth.
  • UserCommonsController.java has 2 PUT endpoints, for buy and sell
  • Buy and Sell Endpoints have USER authorization
  • Mutation testing passes

These are still left for future work

  • PlayPage.js calls backend to update Average Cow Health in onBuy and onSell using given equations. For selling, Cows are sold for user averageCowHealth * cowPrice
  • Make FarmStats.js to display average cow health
  • 100% test coverage for frontend

Details

When a user presses the "+"(onBuy) or "-"(onSell) button on the play page on the website, we call the 2 PUT endpoints /buy and /sell. These buy and sell 1 cow respectively each time they're called. Currently, it sells at cowPrice (value of which comes from when an admin creates a new commons), and buys at cowPrice (although this needs to be changed in a future edit to factor in average cowHealth).

User totalWealth comes from startingBalance, which is set by an admin when creating a new commons.

We created 4 tests for the buy and sell endpoint (2 of which are for checking when the usercommons doesn't exist).

We deleted a line of code from a backend test created by w22-6pm-4 (see commit message regarding it).

Testing Plan

  1. Create a commons
  2. Join the commons
  3. The Total Wealth under Your Farm Stats should be the Starting Wealth entered when you created the commons
  4. When "+" is clicked under Manage Cows, Total Wealth decreases by the Cow Price entered when you created the commons
  5. When "-" is clicked under Manage Cows, Total Wealth decreases by the Cow Price entered when you created the commons

@codecov
Copy link

codecov bot commented Mar 11, 2022

Codecov Report

Merging #51 (92d88b5) into main (b4f5ce2) will increase coverage by 0.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #51      +/-   ##
============================================
+ Coverage     81.09%   81.27%   +0.18%     
- Complexity       75       78       +3     
============================================
  Files            52       49       -3     
  Lines           439      438       -1     
  Branches         12       11       -1     
============================================
  Hits            356      356              
+ Misses           83       82       -1     
Impacted Files Coverage Δ
frontend/src/main/components/Nav/AppNavbar.js 100.00% <ø> (ø)
frontend/src/main/components/OurTable.js 100.00% <ø> (ø)
frontend/src/main/pages/AdminCreateCommonsPage.js 100.00% <ø> (ø)
...d/src/main/components/Commons/CreateCommonsForm.js 100.00% <100.00%> (ø)
frontend/src/main/components/Commons/ManageCows.js 100.00% <100.00%> (ø)
frontend/src/main/pages/PlayPage.js 100.00% <100.00%> (ø)
...156/happiercows/controllers/CommonsController.java 100.00% <100.00%> (ø)
...happiercows/controllers/UserCommonsController.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 b4f5ce2...92d88b5. Read the comment docs.

Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

Where are the controller tests for the new methods?

@pconrad pconrad added Attn-respond-to-CR Please address changes requested in code review Attn-CI/CD-failures At least one of the CI/CD scripts is failing labels Mar 11, 2022
@Lin2 Lin2 changed the title RL-SM: Add Fuctionality for onBuy, onSell Buttons, updating totalWealth using 2 PUT endpoints RL-SM: Add Functionality for onBuy, onSell Buttons, updating totalWealth using 2 PUT endpoints Mar 11, 2022
@Lin2
Copy link
Contributor Author

Lin2 commented Mar 11, 2022

Where are the controller tests for the new methods?

I realized we were missing this right after we made the request -- working on it now!

…e sense to save a usercommons to a nonexistent commons
@Lin2
Copy link
Contributor Author

Lin2 commented Mar 11, 2022

The PR has been completed with controller tests for UserCommonsController.

@Lin2 Lin2 temporarily deployed to team04-w22-6pm-3 March 11, 2022 20:00 Inactive
@eambriz27 eambriz27 temporarily deployed to team04-w22-6pm-3 March 11, 2022 20:06 Inactive
eambriz27
eambriz27 previously approved these changes Mar 12, 2022
Copy link
Contributor

@eambriz27 eambriz27 left a comment

Choose a reason for hiding this comment

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

Looks like you have some merge conflicts to resolve, but everything else LGTM!

@sophiajmoore sophiajmoore force-pushed the rl-sm-hookup-farm-component-to-backend branch from 57897ea to 6bbb27d Compare March 12, 2022 21:04
Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

Couple of "TODOs left in the code"...

when(commonsRepository.findById(eq(1L))).thenReturn(Optional.of(randomCommons));

// act
MvcResult response = mockMvc.perform(put("/api/usercommons/buy?id=1") //oof, accidentally made PUT endpoints id variable instead of commonsId. TODO: Fix later.
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix now maybe?

Comment on lines 264 to 282



















Copy link
Contributor

Choose a reason for hiding this comment

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

As long as we are fixing things, maybe this white space too...

when(commonsRepository.findById(eq(1L))).thenReturn(Optional.of(randomCommons));

// act
MvcResult response = mockMvc.perform(put("/api/usercommons/buy?id=234") //oof, accidentally made PUT endpoints id variable instead of commonsId. TODO: Fix later.
Copy link
Contributor

Choose a reason for hiding this comment

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

oof indeed :-)

@pconrad pconrad added Attn-Merge Conflicts and removed Attn-CI/CD-failures At least one of the CI/CD scripts is failing labels Mar 12, 2022
@sophiajmoore sophiajmoore force-pushed the rl-sm-hookup-farm-component-to-backend branch from 7a14585 to 75b7c14 Compare March 12, 2022 23:49
@Lin2 Lin2 requested a review from eambriz27 March 13, 2022 00:25
@sophiajmoore sophiajmoore temporarily deployed to team04-w22-6pm-3-qa March 13, 2022 00:27 Inactive
@sophiajmoore sophiajmoore temporarily deployed to team04-w22-6pm-3-qa March 13, 2022 00:39 Inactive
@sophiajmoore sophiajmoore temporarily deployed to team04-w22-6pm-3 March 13, 2022 00:43 Inactive
@sophiajmoore sophiajmoore force-pushed the rl-sm-hookup-farm-component-to-backend branch from ad8cc6d to ee26650 Compare March 13, 2022 01:30
eambriz27
eambriz27 previously approved these changes Mar 13, 2022
Copy link
Contributor

@eambriz27 eambriz27 left a comment

Choose a reason for hiding this comment

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

LGTM

@sophiajmoore sophiajmoore temporarily deployed to team04-w22-6pm-3 March 13, 2022 02:51 Inactive
@eambriz27 eambriz27 marked this pull request as draft March 15, 2022 03:04
@Lin2
Copy link
Contributor Author

Lin2 commented Mar 15, 2022

Bad PR

@Lin2 Lin2 closed this Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Attn-Merge Conflicts Attn-respond-to-CR Please address changes requested in code review w22-6pm-3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants