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

Get active mismatches #68

Merged
merged 10 commits into from
Aug 19, 2021
Merged

Get active mismatches #68

merged 10 commits into from
Aug 19, 2021

Conversation

Silvan-WMDE
Copy link
Member

@Silvan-WMDE Silvan-WMDE commented Aug 16, 2021

This modifies the /mismatches endpoint, so that it only returns mismatches that have not been edited yet and that do not belong to expired imports. Two optional request parameters are added, which allow including reviewed mismatches or expired ones.

Bug: T285301

* add new request parameters include_reviewed and include_expired
* adjust tests to new requirements
* modify mismatches response to include status
* modify API spec according to new requirements

Bug: T285301
@Silvan-WMDE Silvan-WMDE marked this pull request as ready for review August 17, 2021 15:05
Copy link
Member

@itamargiv itamargiv left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you, 🙇 Just one thing to change and a couple of suggestions

app/Http/Controllers/MismatchController.php Outdated Show resolved Hide resolved
app/Http/Controllers/MismatchController.php Outdated Show resolved Hide resolved
database/factories/MismatchFactory.php Outdated Show resolved Hide resolved
*
* @return \Illuminate\Database\Eloquent\Factories\Factory
*/
public function reviewed()
Copy link
Member

Choose a reason for hiding this comment

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

I really like the use of states we have in our factories 👍

Comment on lines 28 to 43
Mismatch::factory(10)
->for($import)
->create();

Mismatch::factory(11)
->for($expiredImport)
->create();

Mismatch::factory(10)
->for($import)
->reviewed()
->create();

Mismatch::factory(11)
->for($expiredImport)
->reviewed()
Copy link
Member

Choose a reason for hiding this comment

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

10+11+10+11 = 42 Checks out 🤓 😍

@@ -195,6 +195,14 @@ components:
type: string
external_url:
type: string
status:
type: string
enum:
Copy link
Member

Choose a reason for hiding this comment

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

💯 Thanks for updating the specs!

tests/Feature/ApiMismatchRouteTest.php Outdated Show resolved Hide resolved
Comment on lines 84 to 87
'ids' => $pendingMismatches->implode('item_id', '|') . '|' .
$reviewedMismatches->implode('item_id', '|') . '|' .
$expiredMismatches->implode('item_id', '|') . '|' .
$expiredReviewedMismatches->implode('item_id', '|')
Copy link
Member

Choose a reason for hiding this comment

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

Well constructed!

Copy link
Member

@itamargiv itamargiv left a comment

Choose a reason for hiding this comment

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

Perfect, thank you for all the changes! One last tiny comment and we're good to go!

database/seeders/MismatchSeeder.php Outdated Show resolved Hide resolved
Use private methods to set and get imports, mismatches and their IDs.
@itamargiv
Copy link
Member

Thank you!

@Silvan-WMDE Silvan-WMDE merged commit c204221 into main Aug 19, 2021
@Silvan-WMDE Silvan-WMDE deleted the server/get_active_mismatches branch August 19, 2021 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants