Skip to content
This repository has been archived by the owner on Apr 3, 2021. It is now read-only.

Approve all / select all #157

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

Approve all / select all #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c57cbd5
done
tomahock Aug 12, 2019
61725a3
feat(Entries): Delete all in datatable
franciscobsoares Aug 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 41 additions & 7 deletions app/Http/Controllers/EntriesController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,50 @@ public function add(Request $request)

public function push(Request $request)
{

Copy link
Member

Choose a reason for hiding this comment

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

This blank line can be removed.

$validatedData = $request->validate([
'id' => 'required|exists:entries',
'id' => 'required|array',
Copy link
Member

@quetzyg quetzyg Aug 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is better:

// Must be included for this to work
use Illuminate\Validation\Rule;

// ...

'id.*' => [
    'required',
    'integer',
    Rule::exists('entries', 'id'),
],

Copy link
Member

Choose a reason for hiding this comment

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

Why is it better? Genuinely asking.

Copy link
Member

Choose a reason for hiding this comment

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

Because, reasons.

On a more serious note, the current validation is only checking if the value passed is an array.

We can force that by using id.* instead of id, leaving us to apply validation per item, which in this case we want to make sure is passed (required), is a valid integer number and it exists in the entries table.

]);
try {
$entry = Entry::findOrFail($validatedData['id']);
$entry->push();
return redirect('/panel/entries/list')->with('status', 'Entrada Validada Manualmente!');
} catch (Exception $e) {
return redirect('/panel/entries/list')->with('status', 'Erro ao validar entrada!');

foreach(request()->get('id') as $id){
Copy link
Member

Choose a reason for hiding this comment

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

Use the injected request (i.e. $request) instead of a request() helper.

try {
$entry = Entry::findOrFail($id);
Copy link
Member

@quetzyg quetzyg Aug 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Please move this logic to an EntryService, EntryRepository or whatever class name you want to use, then inject said class into this controller method and run the update() method from here.

$entry->update(['used' => 1]);

$fuel_station = $entry->fuelStation();
Copy link
Member

Choose a reason for hiding this comment

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

Don't use snake_case. Use camelCase for variables and method names, instead. Are the PHP CS Fixer rules being applied?

$fuel_station->update(['has_gasoline' => $entry->has_gasoline,'has_diesel' => $entry->has_diesel, 'has_lpg' => $entry->has_lpg]);
} catch (Exception $e) {
return redirect('/panel/entries/list')->with('status', 'Erro ao validar entrada!');
Copy link
Member

Choose a reason for hiding this comment

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

Show the invalid id in the error message, when redirecting. Otherwise the user has no idea what entry failed.

}
}

$cacheController = new CacheController();
$cacheController->updateStations();
Copy link
Member

Choose a reason for hiding this comment

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

Please, refactor this. Controllers shouldn't be used this way. The logic in the updateStations() method should be moved to a service class, which can then be called from the CacheController and other controllers, but not this way.


return redirect('/panel/entries/list')->with('status', 'Entrada Validada Manualmente!');
}

public function delete(Request $request)
Copy link
Member

Choose a reason for hiding this comment

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

A Delete request class should be created with the validation rules, instead. See this one, for example. The controller will be cleaner with the validation logic in its own place.

{
$validatedData = $request->validate([
'id' => 'required|array',
]);

foreach(request()->get('id') as $id){
try {
\Log::info("deleting ".json_encode($id));
Copy link
Member

Choose a reason for hiding this comment

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

The comments above, also apply in this method.

$entry = Entry::findOrFail($id);
$entry->update(['used' => 1]);

} catch (Exception $e) {
return redirect('/panel/entries/list')->with('status', 'Erro ao validar entrada!');
Copy link
Member

Choose a reason for hiding this comment

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

Since you're making use of translations here, for consistency and better organisation, do the same when passing the status message on redirect.

}
}

$cacheController = new CacheController();
$cacheController->updateStations();

return redirect('/panel/entries/list')->with('status', 'Entrada Validada Manualmente!');
Copy link
Member

@quetzyg quetzyg Aug 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This route has a name set (entries.list). It's good practice to use those, instead of passing the URI.

You probably want to add a name to go along with the panel prefix in the routes, so the route name gets set to: panel.entries.list

}

public function fetch_pending()
Expand Down
6 changes: 3 additions & 3 deletions public/js/map_direct.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,13 @@ function submitEntry(obj, id) {
let gasoline = Number(!($(".mapboxgl-popup-content").find('.gasoline img').hasClass('no-gas')));
let diesel = Number(!($(".mapboxgl-popup-content").find('.diesel img').hasClass('no-gas')));
let lpg = Number(!($(".mapboxgl-popup-content").find('.lpg img').hasClass('no-gas')));
validateCaptcha((token) => {
// validateCaptcha((token) => {
Copy link
Member

Choose a reason for hiding this comment

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

Commented code should be removed.

let data = {
Copy link
Member

Choose a reason for hiding this comment

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

Are we leaving IE users (among others) out on purpose?

https://caniuse.com/#search=let

"fuel_station": id,
"gasoline": gasoline,
"diesel": diesel,
"lpg": lpg,
"captcha": token
// "captcha": token
Copy link
Member

Choose a reason for hiding this comment

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

Commented code should be removed.

}
$(obj).parent().parent().find('.popup_submit_text').html("VALIDADO");
Copy link
Member

Choose a reason for hiding this comment

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

This approach is very dangerous. It is too bounded to markup. If a new layer is added/removed it will break.
Instead we should use a selector to navigate up on the tree to grab the desired element.

https://api.jquery.com/parent/#parent-selector

setTimeout(function() {
Expand All @@ -75,7 +75,7 @@ function submitEntry(obj, id) {
$.post("/panel/entries/add", data, function (reply) {
console.log("Entrada adicionada: " + reply.success + " (0 -> falha, 1 -> sucesso)");
Copy link
Member

Choose a reason for hiding this comment

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

console.log() calls should be removed. If you want to debug while developing, use the debugger construct, instead. And don't forget to remove it, once the code is ready for merging.

}, "json");
});
// });
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

}


Expand Down
45 changes: 41 additions & 4 deletions resources/views/entries/list.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
<h1 class="text-center">{{ __('Entries List') }}</h1>
<div class="row justify-content-center">
<div class="col-md-12">
<p><a href="#" onclick="selectAll()"><i class="fas fa-check"></i> Check all</a></p>
<p><a href="#" onclick="deSelectAll()"><i class="fas fa-check"></i> Uncheck all</a></p>
<p><a href="#" onclick="pushEntries()"><i class="fas fa-check"></i> Aprovar Seleccionados</a></p>
<table id="entry_list" class="table table-striped table-bordered" style="width:100%">
<thead>
<tr>
Expand All @@ -14,6 +17,7 @@
<th>{{ __('Has LPG') }}</th>
<th>{{ __('Count') }}</th>
<th>{{ __('Actions') }}</th>
<th>{{ __('Select') }}</th>
</tr>
</thead>
<tfoot>
Expand All @@ -25,6 +29,7 @@
<th>{{ __('Has LPG') }}</th>
<th>{{ __('Count') }}</th>
<th>{{ __('Actions') }}</th>
<th>{{ __('Select') }}</th>
</tr>
</tfoot>
</table>
Expand All @@ -43,7 +48,7 @@
<div class="modal-footer">
<form method="POST" id="modal_form" class="ui form" action="{{ route('entries.push') }}">
@csrf
<input id="entry_id" type="hidden" name="id" value="0" />
<input class="entry_id" type="hidden" name="id[]" value="0" />
<button type="submit" class="btn btn-primary">Confirmar</button>
</form>
<button type="button" class="btn btn-secondary" data-dismiss="modal">Close</button>
Expand All @@ -59,9 +64,39 @@ function pushEntry(id) {
$('#modal_form').attr('action', '{{ route('entries.push') }}');
$('#action_title').html("Validar Entrada nº"+id);
$('#action_description').html("Esta ação irá validar o a entrada nº"+id+".");
$("#entry_id").val(id);
$(".entry_id").val(id);
$('.modal').modal('show');
}

function pushEntries() {
$selected = $('.select-entry-checkbox:checkbox:checked');
$(".entry_id").remove();

let ids = [];
$selected.each(function(){
$el = $(this);
ids.push($el.data('id'))

$('#modal_form').append('<input class="entry_id" type="hidden" name="id[]" value="'+$el.data('id')+'" />');
});

$('#modal_form').attr('action', '{{ route('entries.push') }}');
$('#action_title').html("Validar Entradas nsº"+ ids.join());
$('#action_description').html("Esta ação irá validar as entradas nº "+ids.join()+".");

$('.modal').modal('show');
}

function selectAll()
{
$('.select-entry-checkbox').prop('checked', true);
}

function deSelectAll()
{
$('.select-entry-checkbox').prop('checked', false);
}

$(document).ready(function() {
$('#entry_list').DataTable( {
"ajax": {
Expand All @@ -87,6 +122,7 @@ function pushEntry(id) {
json.data[index]["has_lpg"] = '<i class="fas fa-times"></i>';
}
json.data[index]["actions"] = '<a href="#" onclick="pushEntry('+json.data[index]["id"]+')"><i class="fas fa-check"></i></a>';
json.data[index]["select"] = '<input type="checkbox" class="select-entry-checkbox" name="checked" data-id="'+json.data[index]["id"]+'"/>';
});
return json.data;
}
Expand All @@ -98,8 +134,9 @@ function pushEntry(id) {
{ "data": "has_diesel" },
{ "data": "has_lpg" },
{ "data": "count" },
{ "data": "actions" }
]
{ "data": "actions" },
{ "data": "select" }
]
});
});

Expand Down
1 change: 1 addition & 0 deletions routes/web.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
Route::get('pending', 'EntriesController@fetch_pending')->name('pending');
});
Route::post('push', 'EntriesController@push')->middleware('auth')->name('push');
Route::post('delete', 'EntriesController@delete')->middleware('auth')->name('delete');
Route::post('add', 'EntriesController@add')->name('add');
});

Expand Down