#2 Station search via device positioning services #79

jheubuch · 2020-08-03T13:07:22Z

This pull request implements the location-based search of the nearest station -> Issue #2

Therefore an extra button was added in the station input field:

When clicking the button the following will happen:

if the user clicks the button the first time, he will be asked to grant the permission to use the positioning services
if the permission was granted and the positioning was successful, the most closest station will be queried and selected for the selections of the trains, buses etc
if the permission was denied, an error message will be displayed

MrKrisKrisu · 2020-08-03T14:34:19Z

This is more a question for the Traewelling team: @HerrLevin @jeyemwey

The variables and function names are partly written in correct CamelCase throughout the whole code (not just this PR), but sometimes with large letters at the beginning. I even found underlined names. It may make sense to standardize this, it doesn't look nice.

MrKrisKrisu · 2020-08-03T14:35:12Z

app/Http/Controllers/FrontendTransportController.php

@@ -42,6 +42,22 @@ public function TrainStationboard(Request $request) {
        );
    }

+    public function StationByCoordinates(Request $request)


I recommend renaming it "getStationByCoordinates", even if other method names are in this format. See my general answer to your pull request for further information.

MrKrisKrisu · 2020-08-03T14:35:35Z

app/Http/Controllers/FrontendTransportController.php

@@ -42,6 +42,22 @@ public function TrainStationboard(Request $request) {
        );
    }

+    public function StationByCoordinates(Request $request)
+    {
+        $NearestStation = TransportBackend::StationByCoordinates($request->latitude, $request->longitude);


I recommend renaming the variable to "$nearestStation"

app/Http/Controllers/TransportController.php

resources/lang/de/stationboard.php

MrKrisKrisu · 2020-08-03T14:40:45Z

resources/views/layouts/app.blade.php

@@ -169,7 +169,7 @@ class="fas fa-tools"></i> {{__('menu.admin')}}</a>
                    <span class="footer-nav-link">/ <a href="{{ route('blog.all') }}">{{ __('menu.blog') }}</a></span>
                </p>
                <p class="mb-0">{!! __('menu.developed') !!}</p>
-                <p class="mb-0">&copy; 2019 Tr&auml;welling</p>
+                <p class="mb-0">&copy; 2020 Tr&auml;welling</p>


Maybe it would be better to change the year to {{date('Y')}} to prevent these changes.
Even if this is not the primary part of the PR.

jeyemwey

Hi @derheubi, thank you soo much for your contribution to Träwelling. I have posted a few remarks, but I'm sure those can be resolved quickly.

Can you (or someone from our core team) please add unit tests for the new Endpoint? I'm especially interested in edge cases like Null Island or the Poles, and far away places like New York, or Beijing. However, it is also possible to add valid tests like Berlin Hbf, Moskau Central or Frankfurt Flughafen Fernbf.

app/Http/Controllers/FrontendTransportController.php

jeyemwey · 2020-08-03T14:54:20Z

routes/web.php

+    Route::get('/trains/nearby', [
+        'uses'  => 'FrontendTransportController@StationByCoordinates',
+        'as'    => 'trains.nearby',
+    ]);
+


Can we add an API request for this? Then we need to adjust the swagger-api.yml too. Maybe @HerrLevin can help? This is non-blocking from my side, and can be done later.

app/Http/Controllers/FrontendTransportController.php

argh. Wrong button

jheubuch · 2020-08-04T07:50:59Z

Can you (or someone from our core team) please add unit tests for the new Endpoint? I'm especially interested in edge cases like Null Island or the Poles, and far away places like New York, or Beijing. However, it is also possible to add valid tests like Berlin Hbf, Moskau Central or Frankfurt Flughafen Fernbf.

The question is, if it is of need to have unit tests for this. It's a core function of the db-rest API. Otherwise the db-rest API needs to be mocked and (I'm sorry :D) I have absolutely no idea how to do this in PHP.

…ith nearby search and switching search time)

MrKrisKrisu · 2020-08-04T08:33:53Z

app/Http/Controllers/FrontendTransportController.php

@@ -44,18 +44,20 @@ public function TrainStationboard(Request $request) {

    public function StationByCoordinates(Request $request)
    {
+        $request->validate([


You have to save the result from ->validate in an variable to use the validated input.

for example:
47: $validated = $request->validate([......
...
52: ....ates($validated['latitude'], $validated['longitude']);

Sorry, my bad.

jeyemwey · 2020-08-04T08:41:19Z

The question is, if it is of need to have unit tests for this. It's a core function of the db-rest API. Otherwise the db-rest API needs to be mocked and (I'm sorry :D) I have absolutely no idea how to do this in PHP.

We do not use mocking for any of the API calls, if db-rest breaks, our CI is going down too. I can add the tests, if you want.

jheubuch · 2020-08-04T08:43:32Z

The question is, if it is of need to have unit tests for this. It's a core function of the db-rest API. Otherwise the db-rest API needs to be mocked and (I'm sorry :D) I have absolutely no idea how to do this in PHP.

We do not use mocking for any of the API calls, if db-rest breaks, our CI is going down too. I can add the tests, if you want.

That would be very nice! Maybe I can do the future upcoming unit tests on my own then 😅

HerrLevin · 2020-08-04T08:47:01Z

Since we're planning to move the frontend to a JS-based API-client in the future, could you maybe implement this as an API-endpoint?
If not, that's no problem. It's just easier for us. 😅

jheubuch · 2020-08-04T08:48:57Z

Since we're planning to move the frontend to a JS-based API-client in the future, could you maybe implement this as an API-endpoint?
If not, that's no problem. It's just easier for us. 😅

For sure! I was looking at this topic anyway (it was mentioned by @jeyemwey in a review above)

jheubuch · 2020-08-04T09:44:00Z

@HerrLevin I tried adding an API endpoint but actually I had no chance to test it (I get an error when sending an API request):

I also added the endpoint to the api-swagger-v0.yml (swagger.io says, the configuration is valid)

…station details

jeyemwey · 2020-08-04T09:55:25Z

I have written some tests now for the frontend. @derheubi, can you check the "Allow edits from maintainers" checkbox? https://tighten.co/blog/adding-commits-to-a-pull-request/

jheubuch · 2020-08-04T09:56:22Z

I have written some tests now for the frontend. @derheubi, can you check the "Allow edits from maintainers" checkbox? https://tighten.co/blog/adding-commits-to-a-pull-request/

Done

jeyemwey

Looks good to me now.

MrKrisKrisu · 2020-08-04T10:15:07Z

Before merging: Can we speak about my comments about unified conventions?
@derheubi has used CamelCase Names which started with an uppercase letter. In the whole traewelling code there is no unified procedure for this. Sometimes there is a "getObject" or "ObjectName" Identifier.

It's non blocking for me here, but it needs to be discussed.

//Edit: Okay, 20 seconds too late. Merged. xD

jeyemwey · 2020-08-04T10:17:28Z

Sorry 😂 I have resolved tha sometimes, so there are only few issues left.

jheubuch added 2 commits August 3, 2020 12:43

Station search by location

fdcc9f9

Add error message for denied gps permission or positioning errors

8a117f2

MrKrisKrisu requested changes Aug 3, 2020

View reviewed changes

jeyemwey previously approved these changes Aug 3, 2020

View reviewed changes

jeyemwey self-requested a review August 3, 2020 15:03

jheubuch added 3 commits August 4, 2020 10:15

Change routing to Laravel route-function in stationboard (fixes bug w…

92dedd8

…ith nearby search and switching search time)

Add build-and-run node-script for npm start

9287960

Request validation and redirecting to Stationboard route, typo fix

3f64c21

MrKrisKrisu requested changes Aug 4, 2020

View reviewed changes

Use input validation for backend call

4df0f05

MrKrisKrisu approved these changes Aug 4, 2020

View reviewed changes

Add API endpoint for location based station search

3c95bf4

jheubuch and others added 3 commits August 4, 2020 11:46

copy-paste-mistake :(

9422f84

Add: Tests for the Frontend Endpoint

c3b7caa

Add tests and stop pulling the stationboard api then we only use the …

906a985

…station details

jeyemwey approved these changes Aug 4, 2020

View reviewed changes

jeyemwey merged commit fcc4943 into Traewelling:develop Aug 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2 Station search via device positioning services #79

#2 Station search via device positioning services #79

jheubuch commented Aug 3, 2020

MrKrisKrisu commented Aug 3, 2020

MrKrisKrisu Aug 3, 2020

MrKrisKrisu Aug 3, 2020

MrKrisKrisu Aug 3, 2020

jeyemwey left a comment

jeyemwey Aug 3, 2020

jheubuch commented Aug 4, 2020

MrKrisKrisu Aug 4, 2020

jeyemwey Aug 4, 2020

jeyemwey commented Aug 4, 2020

jheubuch commented Aug 4, 2020

HerrLevin commented Aug 4, 2020

jheubuch commented Aug 4, 2020

jheubuch commented Aug 4, 2020

jeyemwey commented Aug 4, 2020

jheubuch commented Aug 4, 2020

jeyemwey left a comment

MrKrisKrisu commented Aug 4, 2020 •

edited

jeyemwey commented Aug 4, 2020

#2 Station search via device positioning services #79

#2 Station search via device positioning services #79

Conversation

jheubuch commented Aug 3, 2020

MrKrisKrisu commented Aug 3, 2020

MrKrisKrisu Aug 3, 2020

Choose a reason for hiding this comment

MrKrisKrisu Aug 3, 2020

Choose a reason for hiding this comment

MrKrisKrisu Aug 3, 2020

Choose a reason for hiding this comment

jeyemwey left a comment

Choose a reason for hiding this comment

jeyemwey Aug 3, 2020

Choose a reason for hiding this comment

jheubuch commented Aug 4, 2020

MrKrisKrisu Aug 4, 2020

Choose a reason for hiding this comment

jeyemwey Aug 4, 2020

Choose a reason for hiding this comment

jeyemwey commented Aug 4, 2020

jheubuch commented Aug 4, 2020

HerrLevin commented Aug 4, 2020

jheubuch commented Aug 4, 2020

jheubuch commented Aug 4, 2020

jeyemwey commented Aug 4, 2020

jheubuch commented Aug 4, 2020

jeyemwey left a comment

Choose a reason for hiding this comment

MrKrisKrisu commented Aug 4, 2020 • edited

jeyemwey commented Aug 4, 2020

MrKrisKrisu commented Aug 4, 2020 •

edited