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

Trade index #395

Merged
merged 17 commits into from
Feb 19, 2022
Merged

Trade index #395

merged 17 commits into from
Feb 19, 2022

Conversation

bcobo341
Copy link
Contributor

@bcobo341 bcobo341 commented Feb 11, 2022

Description

Filtered trades by requested_items, description, item name, item description, or item rarity.
Closes #376

@bcobo341 bcobo341 added enhancement New feature or request back end labels Feb 11, 2022
@bcobo341 bcobo341 added this to the Sprint #7 milestone Feb 11, 2022
@codeclimate
Copy link

codeclimate bot commented Feb 11, 2022

Code Climate has analyzed commit 85e1ab1 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (90% is the threshold).

This pull request will bring the total coverage in the repository to 91.2% (0.1% change).

View more on Code Climate.

Copy link
Contributor

@m-triassi m-triassi left a comment

Choose a reason for hiding this comment

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

its a good start! just needs some adjusting :)

app/Http/Controllers/TradeController.php Outdated Show resolved Hide resolved
app/Http/Controllers/TradeController.php Outdated Show resolved Hide resolved
app/Models/Trade.php Outdated Show resolved Hide resolved
Comment on lines 42 to 49
$response = $this->get(route('trade.index', [
'requested_items' => $trades->first()->requested_items,
'description' => $trades->first()->description,
'item_name' => $trades->first()->items->first()->name,
'item_description' => $trades->first()->items->first()->description,
'item_rarity' => $trades->first()->items->first()->rarity,
]));

Copy link
Contributor

Choose a reason for hiding this comment

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

there aren't any assertions in this test, would you be able to add some that check the trade items are there? Or maybe that the count of items returned in the trade the expected amount of items based off the applied filters?

Comment on lines 38 to 40
$trades = Trade::factory(3)
->has(Item::factory(3))
->create();
Copy link
Contributor

Choose a reason for hiding this comment

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

This isnt the state a trade would be in; basically a trade will post an item and have offers in the items relation (note the s) so you'd need to define the specific relationships your assigning the items too, you can find how that works here in the docs, note the second code block where they specify the relationship name

app/Http/Controllers/TradeController.php Outdated Show resolved Hide resolved
app/Http/Controllers/TradeController.php Outdated Show resolved Hide resolved
@bcobo341 bcobo341 marked this pull request as ready for review February 18, 2022 03:52
@sonarcloud
Copy link

sonarcloud bot commented Feb 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@m-triassi m-triassi left a comment

Choose a reason for hiding this comment

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

This looks pretty excellent to me!

Comment on lines +58 to +78
fn (Assert $page) => $page
->where('id', $trades->first()->id)
->where('item_id', $trades->first()->item_id)
->where('character_id', $trades->first()->character_id)
->where('requested_items', $trades->first()->requested_items)
->where('description', $trades->first()->description)
->where('status', $trades->first()->status)
->etc()
->has(
'item',
fn (Assert $page) => $page
->where('id', $trades->first()->item->id)
->where('entry_id', $trades->first()->item->entry_id)
->where('character_id', $trades->first()->item->character_id)
->where('name', $trades->first()->item->name)
->where('author_id', $trades->first()->item->author_id)
->where('rarity', $trades->first()->item->rarity)
->where('tier', $trades->first()->item->tier)
->where('description', $trades->first()->item->description)
->etc()
)
Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow, nice, this is really proper testing with inertia responses; good work!

@m-triassi m-triassi merged commit 80c6518 into main Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back end enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trade Index
2 participants