Skip to content
This repository has been archived by the owner on Nov 26, 2024. It is now read-only.

selling cards functionality #116

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

selling cards functionality #116

wants to merge 2 commits into from

Conversation

larafa
Copy link
Contributor

@larafa larafa commented Apr 26, 2018

-Cards to be sold are added back to marketplace with the new price and removed from user's list of cards

@codecov-io
Copy link

codecov-io commented Apr 26, 2018

Codecov Report

Merging #116 into master will decrease coverage by 1.62%.
The diff coverage is 5.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #116      +/-   ##
============================================
- Coverage     23.58%   21.95%   -1.63%     
- Complexity      133      147      +14     
============================================
  Files            91       93       +2     
  Lines          1463     1635     +172     
  Branches        188      208      +20     
============================================
+ Hits            345      359      +14     
- Misses         1042     1197     +155     
- Partials         76       79       +3
Impacted Files Coverage Δ Complexity Δ
frontend/src/components/CardValue.js 18.18% <ø> (+18.18%) 0 <0> (ø) ⬇️
frontend/src/pages/UserDetail.js 3.03% <ø> (ø) 0 <0> (ø) ⬇️
frontend/src/actions/cards.js 14.81% <ø> (ø) 0 <0> (ø) ⬇️
frontend/src/pages/CardDetail.js 2.7% <0%> (ø) 0 <0> (ø) ⬇️
frontend/src/actions/listings.js 8.69% <0%> (-6.69%) 0 <0> (ø)
frontend/src/components/SellCards.js 15.38% <15.38%> (ø) 0 <0> (?)
...end/app/Http/Controllers/MarketplaceController.php 22.89% <4.16%> (-17.54%) 23 <5> (+10)
backend/app/Http/Controllers/ProfileController.php 63.07% <0%> (-12.85%) 24% <0%> (+4%)
frontend/src/reducers/reducer_user.js 0% <0%> (ø) 0% <0%> (ø) ⬇️
... and 5 more

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 41bac36...ebb3a81. Read the comment docs.


export function sellCards(cardIds) {
console.log('cardss');
console.log(cardIds);
Copy link
Contributor

Choose a reason for hiding this comment

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

should remove console.log statements

@bbemis017 bbemis017 requested a review from nickysemenza April 26, 2018 18:55
//add selling car to listing
$user = auth()->user();
$data = json_decode(Request::getContent(), true);
foreach ($data as $cardId) {
Copy link
Member

Choose a reason for hiding this comment

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

Do not assume that the user passes a valid set of card IDs.

We need to verify that:

  • The card exists
  • The card is available for auction (not already on auction, not in battle, etc)
  • The card is owned by the user trying to sell the card (thus the $listing->user_id should not be set to the $user->id

$listing->price = $this->estimateValue($cardId);
$listing->save();

//remove selling card from user's cards
Copy link
Member

Choose a reason for hiding this comment

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

The card should not be removed from the user until the Listing has found a buyer, at which point the card will be removed from the old user and added to the new user instantaneously.

@@ -27,13 +27,15 @@

Route::get('/battles', 'BattlegroundController@getAllBattles');

Route::put('/sell', 'MarketplaceController@sellCards');
Copy link
Member

Choose a reason for hiding this comment

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

Why does sell take an array of cardIDs and not just a single cardID parameter to sell (like all other function calls)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because in the frontend the user selects one or more cards to sell so the cards are passed to the backend together. I think battles does it the same way in the contract controller

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants