fix: Address PR review findings — CSRF, status guards, SQL safety, transactions
- Add CSRF token validation on EstimateView approve/reject forms - Add status guard on reject branch (prevent reversing approved estimates) - Fix loc.* column collision in WorkOrdersModel::getWorkOrder (overwrote wo.id) - Add (int) cast on all query parameter concatenations - Wrap InvoiceHelper::generateFromWorkOrder in database transaction - Replace magic 0.5 divisor with AVG_SPEED_MPH constant in RouteHelper
This commit is contained in:
@@ -38,13 +38,14 @@ class WorkOrdersModel extends BaseDatabaseModel
|
||||
{
|
||||
$db = $this->getDatabase();
|
||||
$db->setQuery($db->getQuery(true)
|
||||
->select('wo.*, cd.name AS customer_name, loc.*, t_cd.name AS tech_name')
|
||||
->select('wo.*, cd.name AS customer_name, t_cd.name AS tech_name')
|
||||
->select('loc.address, loc.city, loc.state, loc.zip, loc.latitude, loc.longitude, loc.name AS location_name')
|
||||
->from($db->quoteName('#__mokosuitefield_work_orders', 'wo'))
|
||||
->join('LEFT', $db->quoteName('#__contact_details', 'cd') . ' ON cd.id = wo.contact_id')
|
||||
->join('LEFT', $db->quoteName('#__mokosuitefield_locations', 'loc') . ' ON loc.id = wo.location_id')
|
||||
->join('LEFT', $db->quoteName('#__mokosuitefield_technicians', 't') . ' ON t.id = wo.technician_id')
|
||||
->join('LEFT', $db->quoteName('#__contact_details', 't_cd') . ' ON t_cd.id = t.contact_id')
|
||||
->where('wo.id = ' . $id));
|
||||
->where('wo.id = ' . (int) $id));
|
||||
return $db->loadObject();
|
||||
}
|
||||
|
||||
|
||||
@@ -55,8 +55,8 @@ class HtmlView extends BaseHtmlView
|
||||
->order('ordering ASC'));
|
||||
$this->lineItems = $db->loadObjectList() ?: [];
|
||||
|
||||
// Handle approval/rejection
|
||||
if ($input->getMethod() === 'POST') {
|
||||
// Handle approval/rejection (CSRF check not required — token-based public page)
|
||||
if ($input->getMethod() === 'POST' && \Joomla\CMS\Session\Session::checkToken()) {
|
||||
$action = $input->getString('action', '');
|
||||
|
||||
if ($action === 'approve' && $this->estimate->status === 'sent') {
|
||||
@@ -71,7 +71,7 @@ class HtmlView extends BaseHtmlView
|
||||
$this->actioned = true;
|
||||
$this->actionResult = 'approved';
|
||||
$this->estimate->status = 'approved';
|
||||
} elseif ($action === 'reject') {
|
||||
} elseif ($action === 'reject' && $this->estimate->status === 'sent') {
|
||||
$db->setQuery($db->getQuery(true)
|
||||
->update('#__mokosuitefield_estimates')
|
||||
->set($db->quoteName('status') . ' = ' . $db->quote('rejected'))
|
||||
|
||||
@@ -61,6 +61,7 @@ $total = $subtotal + $tax;
|
||||
<div class="row g-3">
|
||||
<div class="col-md-6">
|
||||
<form method="post">
|
||||
<?php echo \Joomla\CMS\HTML\HTMLHelper::_('form.token'); ?>
|
||||
<input type="hidden" name="action" value="approve">
|
||||
<div class="card border-success">
|
||||
<div class="card-body">
|
||||
@@ -76,6 +77,7 @@ $total = $subtotal + $tax;
|
||||
</div>
|
||||
<div class="col-md-6">
|
||||
<form method="post">
|
||||
<?php echo \Joomla\CMS\HTML\HTMLHelper::_('form.token'); ?>
|
||||
<input type="hidden" name="action" value="reject">
|
||||
<div class="card border-danger">
|
||||
<div class="card-body">
|
||||
|
||||
@@ -18,6 +18,7 @@ class InvoiceHelper
|
||||
{
|
||||
$db = Factory::getContainer()->get(DatabaseInterface::class);
|
||||
$now = Factory::getDate()->toSql();
|
||||
$woId = (int) $woId;
|
||||
|
||||
// Load work order
|
||||
$db->setQuery($db->getQuery(true)
|
||||
@@ -29,6 +30,8 @@ class InvoiceHelper
|
||||
|
||||
if (!$wo) throw new \RuntimeException('Work order not found: ' . $woId);
|
||||
|
||||
$db->transactionStart();
|
||||
|
||||
$params = Factory::getApplication()->getParams('com_mokosuitefield');
|
||||
$laborRate = (float) $params->get('default_labor_rate', 85.00);
|
||||
|
||||
@@ -113,6 +116,8 @@ class InvoiceHelper
|
||||
->where('id = ' . $woId));
|
||||
$db->execute();
|
||||
|
||||
$db->transactionCommit();
|
||||
|
||||
return $invoiceId;
|
||||
}
|
||||
|
||||
|
||||
@@ -11,6 +11,8 @@ use Joomla\Database\DatabaseInterface;
|
||||
*/
|
||||
class RouteHelper
|
||||
{
|
||||
private const AVG_SPEED_MPH = 30;
|
||||
|
||||
/**
|
||||
* Get today's route for a technician (work orders sorted by scheduled time).
|
||||
*/
|
||||
@@ -185,14 +187,14 @@ class RouteHelper
|
||||
'wo_id' => $stop->id,
|
||||
'location' => $stop->location_name ?? $stop->address,
|
||||
'distance' => round($dist, 1),
|
||||
'drive_min'=> round($dist / 0.5, 0), // ~30 mph avg in service areas
|
||||
'drive_min'=> round($dist / self::AVG_SPEED_MPH * 60, 0),
|
||||
];
|
||||
|
||||
$prevLat = $lat ?: $prevLat;
|
||||
$prevLng = $lng ?: $prevLng;
|
||||
}
|
||||
|
||||
$totalDriveMin = $totalDistance > 0 ? round($totalDistance / 0.5) : 0;
|
||||
$totalDriveMin = $totalDistance > 0 ? round($totalDistance / self::AVG_SPEED_MPH * 60) : 0;
|
||||
|
||||
return (object) [
|
||||
'stop_count' => count($stops),
|
||||
|
||||
Reference in New Issue
Block a user