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

Add stats in User resource API #1900

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from
Open

Add stats in User resource API #1900

wants to merge 25 commits into from

Conversation

rcomunica
Copy link
Contributor

Hello Nabel!

I'm here again, I think create a project that maybe can help for all community and I expect more later tell you about this.
But firts I need make some modifications in the API's

Preview

Endpoint: /api/user

{
       ...
        "flights": 125,
        "flight_time": 12463,
        "transfer_time": 0,
        "total_time": 12463,
        "timezone": "GMT",
        "state": 1,
        "stats": {
            "balance": 9999.99,
            "avgScore": "100",
            "avgLanding": "-100",
            "avgFuel": "4,648 kg"
        },
        ...
}

Remember that only you can user Average Score and Average Landing if your pirep was submitted with Acars

I hope in the future add more stats how Average burn fuel and other field

@nabeelio
Copy link
Owner

Hi @rcomunica!

Thanks for this. It looks good, however, I think you need to put the stats under its own controller and endpoint. Something like:

GET /api/user/<id>/stats
app/Http/Controllers/Api/StatsController.php

This way we can expand on them. The DB calls in the Resource aren't the right place. Balance should be returned already, and the naming of the return fields should be in snake_case

@rcomunica
Copy link
Contributor Author

Okay @nabeelio I'll make a commit you again when the features are ready

@rcomunica
Copy link
Contributor Author

Hello @nabeelio,

I think it's ready!... and sorry for the delay, I didn't have internet.

Copy link
Contributor

@arthurpar06 arthurpar06 left a comment

Choose a reason for hiding this comment

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

Hey,
Here are some thoughts to make your code more readable and more aligned with the repository structure.

The StatsController.php file should be in the App\Http\Controllers\Api directory rather than App\Http\Resources since this is a controller and this controller should be used by the api.

It also looks like you forgot to register the route, which means that even though the code is present, no one will be able to access it. You need to add a route to the RouteServiceProvider like this. Once added, you'll be able to test it and see if it works correctly.

https://github.com/nabeelio/phpvms/blob/dev/app/Providers/RouteServiceProvider.php#L662

Route::get('user/stats', 'StatsController@index');

app/Http/Resources/StatsController.php Outdated Show resolved Hide resolved
app/Http/Resources/StatsController.php Outdated Show resolved Hide resolved
app/Http/Resources/StatsController.php Outdated Show resolved Hide resolved
app/Http/Resources/StatsController.php Outdated Show resolved Hide resolved
@rcomunica
Copy link
Contributor Author

Hey, Here are some thoughts to make your code more readable and more aligned with the repository structure.

The StatsController.php file should be in the App\Http\Controllers\Api directory rather than App\Http\Resources since this is a controller and this controller should be used by the api.

It also looks like you forgot to register the route, which means that even though the code is present, no one will be able to access it. You need to add a route to the RouteServiceProvider like this. Once added, you'll be able to test it and see if it works correctly.

https://github.com/nabeelio/phpvms/blob/dev/app/Providers/RouteServiceProvider.php#L662

Route::get('user/stats', 'StatsController@index');

Yea man!, thank you. Really I forgot create the folders and other files in github, also upload the file RouteServiceProvider, but thanks!

'average_time' => $this->average_flight_time,
'average_score' => number_format($this->average_score),
'balance' => $this->balance,
'average_fuel' => number_format($this->average_fuel_used / 2.20462262185).' Kg',
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think that this is correct. It should be casted as Fuel and then we can get both lbs and kg results.

Forcing something to kg is not logical 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like below should work

use App\Support\Units\Fuel

'average_fuel' => Fuel::make($this->average_fuel_used, config('phpvms.internal_units.fuel'))->getResponseUnits();

$response['average_'.$static] = Pirep::where($where)->avg($static);
}

$response['balance'] = Auth::user()->journal->balance->getValue() ?? 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't want to create extra queries, you should eager load the journal relationship here.

Also no need to call the Auth facade twice, you can get the user once with relationships and use wherever needed 😉

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

Successfully merging this pull request may close these issues.

4 participants