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.

Sign up for GitHub

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

Jump to bottom

Taeho-fixed cow bought/sold bug #34

Merged
merged 6 commits into from Nov 30, 2022
Merged

Taeho-fixed cow bought/sold bug #34

merged 6 commits into from Nov 30, 2022

Conversation

taeho1031
Copy link
Contributor

@taeho1031 taeho1031 commented Nov 27, 2022 •
edited

In this PR, I modified the frontend and backend so that error message occurs when user tries to buy a cow without enough money or user tries to sell a cow without any cow.

@jcucsb jcucsb added the f22-6pm-3 f22-6pm-3 label Nov 27, 2022
@jcucsb jcucsb temporarily deployed to f22-6pm-3-happycows-qa November 27, 2022 05:45 Inactive
@pconrad pconrad added the FIXME: Peer CR Please get a peer code review label Nov 27, 2022
jcucsb
jcucsb reviewed Nov 27, 2022
View reviewed changes
Copy link
Contributor

@jcucsb jcucsb left a comment

Choose a reason for hiding this comment

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

I think there needs to be one small change implemented. Deployed on Heroku and tested and it seems that when I buy a cow I still get a toast that says "Cow Sold". I think that in the issue it says a toast should only appear when you don't have any cows left to sell. Other than that it works fine when you don't have any cows left to sell. Currently passing CI/CD checks.

@sergioacolis sergioacolis temporarily deployed to f22-6pm-3-happycows-qa November 28, 2022 20:35 Inactive
@taeho1031 taeho1031 temporarily deployed to f22-6pm-3-happycows-qa November 29, 2022 00:03 Inactive
@taeho1031 taeho1031 changed the title Taeho-fixed cow sold bug Taeho-fixed cow bought/sold bug Nov 29, 2022
@pconrad
Copy link
Contributor

pconrad commented Nov 29, 2022

frontend run coverage failing with this (and only this)

image

@pconrad
Copy link
Contributor

pconrad commented Nov 29, 2022

34-frontend-mutation-testing failing with this and only this, which amounts to the same thing:

image

Since that's unrelated to your PR, I'll allow it for now.. but we need to get this fixed.

I'm hopeful that PR #31 from the -4 team might address this... we'll see.

@pconrad pconrad added the FIXME: Changes Requested Please address the changes specified in the code review label Nov 29, 2022
@jcucsb jcucsb temporarily deployed to f22-6pm-3-happycows-qa November 29, 2022 01:03 Inactive
pconrad
pconrad requested changes Nov 29, 2022
View reviewed changes
frontend/src/main/pages/PlayPage.js Outdated Show resolved Hide resolved
frontend/src/main/pages/PlayPage.js Outdated Show resolved Hide resolved
@sergioacolis sergioacolis temporarily deployed to f22-6pm-3-happycows-qa November 29, 2022 20:27 Inactive
@jcucsb jcucsb temporarily deployed to f22-6pm-3-happycows-qa November 29, 2022 21:36 Inactive
jcucsb
jcucsb previously approved these changes Nov 29, 2022
View reviewed changes
Copy link
Contributor

@jcucsb jcucsb left a comment •
edited

Choose a reason for hiding this comment

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

Review did not mention deleting commented lines of code.

@sergioacolis sergioacolis added STAFF: ready for review green on CI, has peer CR, no other obvious issues and removed FIXME: Peer CR Please get a peer code review FIXME: Changes Requested Please address the changes specified in the code review labels Nov 29, 2022
jcucsb
jcucsb previously approved these changes Nov 30, 2022
View reviewed changes
Copy link
Contributor

@jcucsb jcucsb left a comment •
edited

Choose a reason for hiding this comment

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

Updated commented out code

jcucsb
jcucsb approved these changes Nov 30, 2022
View reviewed changes
Copy link
Contributor

@jcucsb jcucsb left a comment

Choose a reason for hiding this comment

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

Commented out code removed, passes all CI/CD tests, LGTM

@pconrad pconrad temporarily deployed to f22-6pm-3-happycows November 30, 2022 22:52 Inactive
@pconrad pconrad added 10 10 pts and removed STAFF: ready for review green on CI, has peer CR, no other obvious issues labels Nov 30, 2022
pconrad
pconrad approved these changes Nov 30, 2022
View reviewed changes
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.

LGTM

frontend/src/main/pages/PlayPage.js Outdated
@@ -1,7 +1,7 @@
import React from "react";
import { Container, CardGroup } from "react-bootstrap";
import { useParams } from "react-router-dom";
import { toast } from "react-toastify";
//import { toast } from "react-toastify";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove commented out code.

@pconrad pconrad merged commit c64a195 into main Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pconrad pconrad pconrad approved these changes

@jcucsb jcucsb jcucsb approved these changes

Assignees
No one assigned
Labels
10 10 pts f22-6pm-3 f22-6pm-3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@taeho1031 @pconrad @jcucsb @sergioacolis