Add service for a company (replace #1020) #1372
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application/modules/invoices/models/Mdl_invoices.php (1)
243-261: Remove incorrect 'services' field from item db_array.Line 261 adds a
'services'field to the item's$db_array, which is then passed to$this->mdl_items->save(). This field doesn't belong in the items table and will likely cause a database error or be silently ignored.Items should only have a
service_idreference, not the entire services array.🔎 Proposed fix
// Copy the items $invoice_items = $this->mdl_items->where('invoice_id', $source_id)->get()->result(); - $services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); - foreach ($invoice_items as $invoice_item) { $db_array = [ 'invoice_id' => $target_id, 'service_id' => $invoice_item->service_id, 'item_tax_rate_id' => $invoice_item->item_tax_rate_id, 'item_product_id' => $invoice_item->item_product_id, 'item_task_id' => $invoice_item->item_task_id, 'item_name' => $invoice_item->item_name, 'item_description' => $invoice_item->item_description, 'item_quantity' => $invoice_item->item_quantity, 'item_price' => $invoice_item->item_price, 'item_discount_amount' => $invoice_item->item_discount_amount, 'item_order' => $invoice_item->item_order, 'item_is_recurring' => $invoice_item->item_is_recurring, 'item_product_unit' => $invoice_item->item_product_unit, 'item_product_unit_id' => $invoice_item->item_product_unit_id, - 'services' => $services, ];application/modules/invoices/controllers/Ajax.php (1)
133-144: Fix the syntax error: missing closing parenthesis.Line 143 is missing a closing parenthesis, which will cause a PHP parse error and prevent the invoice save functionality from working.
🔎 Proposed fix
'invoice_discount_percent' => standardize_amount($invoice_discount_percent), - 'service_id' => $this->security->xss_clean($this->input->post('service_id'), + 'service_id' => $this->security->xss_clean($this->input->post('service_id')), ];
🟠 Major comments (10)
application/modules/clients/controllers/Clients.php-201-222 (1)
201-222: Remove commented-out dead code.This large block of commented code (22 lines) should be removed rather than left in the codebase. If this logic is needed, it should be implemented properly; otherwise, it clutters the file and reduces maintainability.
application/modules/setup/sql/038_1.6.x.sql-7-13 (1)
7-13: Use NULL for optional foreign key columns instead of DEFAULT 0.Using
DEFAULT 0for foreign key columns (service_id) is not recommended practice. When a service is not assigned, the value should beNULLrather than0, which may not correspond to a valid service. Additionally, consider adding foreign key constraints to maintain referential integrity.🔎 Recommended changes
ALTER TABLE `ip_invoices` - ADD `service_id` int(11) DEFAULT 0 + ADD `service_id` int(11) DEFAULT NULL, + ADD CONSTRAINT fk_invoices_service FOREIGN KEY (service_id) REFERENCES ip_services(service_id) ON DELETE SET NULL AFTER `creditinvoice_parent_id`; ALTER TABLE `ip_quotes` - ADD `service_id` int(11) DEFAULT 0 + ADD `service_id` int(11) DEFAULT NULL, + ADD CONSTRAINT fk_quotes_service FOREIGN KEY (service_id) REFERENCES ip_services(service_id) ON DELETE SET NULL AFTER `notes`;Committable suggestion skipped: line range outside the PR's diff.
application/modules/invoices/views/modal_copy_invoice.php-75-92 (1)
75-92: Address multiple issues in the service dropdown.
- Conflicting autofocus: Both
client_id(line 67) andservice_id(line 79) haveautofocus="autofocus". Only one element can receive autofocus.- Inline styles: Using inline
style="width: 100%;"is inconsistent with the rest of the modal where classes are used.- Missing validation: No check that
$servicesis defined and is an array before theforeachloop.- Inconsistent indentation: Mix of tabs and spaces throughout this section.
🔎 Proposed fixes
<div class="form-group has-feedback"> <label for="service_id"><?php _trans('service'); ?></label> - <div class="input-group" style="width: 100%;"> - <select name="service_id" id="service_id" class="form-control" style="width: 100%;" - autofocus="autofocus"> + <div class="input-group"> + <select name="service_id" id="service_id" class="form-control simple-select"> <option value="0" selected><?php _trans('select_service'); ?></option> <?php + if (!empty($services)) { foreach($services as $service) { if ($service['service_name']) { $value = $service['service_name']; echo '<option value="' . $service['service_id'] .'">' . $value .'</option>'; } } + } ?> </select> </div> </div>application/modules/clients/models/Mdl_clients.php-314-318 (1)
314-318: Misleading method name and SQL anti-pattern.The method
service_by_client()suggests it filters services by a specific client, but it actually returns all services from the database without any client-specific filtering:
Misleading name: The method doesn't accept a
client_idparameter and doesn't filter results by client. Consider renaming toget_all_services()or similar.SQL anti-pattern:
WHERE 1is unnecessary. If you need a condition placeholder for dynamic queries, use the query builder's conditional methods. For a static query returning all rows, omit the WHERE clause entirely.Missing documentation: No PHPDoc describing the method's purpose, return type, or usage.
🔎 Proposed fix
- public function service_by_client () + /** + * Get all services ordered by name + * + * @return array Array of services with service_id and service_name + */ + public function get_all_services() { - $query = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name'); - return $query->result_array(); + return $this->db + ->select('service_id, service_name') + ->from('ip_services') + ->order_by('service_name', 'ASC') + ->get() + ->result_array(); }Note: You'll also need to update the call site in
application/modules/invoices/controllers/Ajax.php(and any other locations) to use the new method name.application/modules/payments/controllers/Payments.php-37-43 (1)
37-43: Potential N+1 query performance issue.The code executes a database query inside a loop for each payment, which could lead to performance degradation when displaying many payments. Consider fetching all required service names in a single query or using a JOIN in the main payments query.
🔎 Proposed refactor to eliminate N+1 queries
Instead of querying services in the loop, modify the model's default_join to include the service:
In
application/modules/payments/models/Mdl_payments.php(if it exists), add:public function default_join() { $this->db->join('ip_invoices', 'ip_invoices.invoice_id = ip_payments.invoice_id', 'left'); $this->db->join('ip_services', 'ip_services.service_id = ip_invoices.service_id', 'left'); } public function default_select() { $this->db->select('ip_payments.*, ip_services.service_name'); }Then remove the loop in the controller:
$this->mdl_payments->paginate(site_url('payments/index'), $page); $payments = $this->mdl_payments->result(); - foreach ($payments as $payment) { - $service = $this->db->query('SELECT service_name FROM ip_services, ip_invoices WHERE ip_services.service_id = ip_invoices.service_id AND ip_invoices.invoice_id = ?', $payment->invoice_id)->result_array(); - if ($service && $service[0] && $service[0]['service_name']) - $payment->service_name = $service[0]['service_name']; - else - $payment->service_name = null; - } -Committable suggestion skipped: line range outside the PR's diff.
application/modules/invoices/views/view.php-426-449 (1)
426-449: Add null safety check for $services array.The
$servicesarray is used in the foreach loop without a null or empty check. If the controller fails to populate this variable, a runtime error will occur.🔎 Proposed fix
<div class="client-address"> <label for="service_id"> <?php echo ' <span class="small">(' . trans('service_name') . ')</span>'; ?> </label> <select name="service_id" id="service_id" class="form-control input-sm simple-select" data-minimum-results-for-search="Infinity" <?php if ($invoice->is_read_only == 1 && $invoice->invoice_status_id == 4) { echo 'disabled="disabled"'; } ?>> <option value="0" selected><?php _trans('select_service'); ?></option> <?php + if (isset($services) && is_array($services)) { foreach($services as $service) { if ($service['service_name']) { echo '<option value="' . $service['service_id'] .'" '; if ($service['service_id'] == $invoice->service_id) echo 'selected'; echo '>' . $service['service_name'] .'</option>'; } } + } ?> </select><br>application/modules/payments/controllers/Payments.php-136-141 (1)
136-141: Potential N+1 query performance issue.Similar to the index() method, this code executes a database query for each open invoice in the loop. Consider using a JOIN or a single query with WHERE IN to fetch all service names at once.
🔎 Proposed refactor
Fetch all services once and create a lookup map:
$open_invoices = $this->mdl_invoices->is_open()->get()->result(); + // Fetch all services at once + $service_ids = array_unique(array_column($open_invoices, 'service_id')); + $services_result = $this->db->where_in('service_id', $service_ids) + ->get('ip_services') + ->result_array(); + $services_map = []; + foreach ($services_result as $service) { + $services_map[$service['service_id']] = $service['service_name']; + } + foreach ($open_invoices as $open_invoice) { - $service = $this->db->query('SELECT service_name FROM ip_services WHERE service_id = ?', $open_invoice->service_id)->result_array(); - if ($service && $service[0] && $service[0]['service_name']) - $open_invoice->service_name = $service[0]['service_name']; - else - $open_invoice->service_name = null; + $open_invoice->service_name = $services_map[$open_invoice->service_id] ?? null; $amounts['invoice' . $open_invoice->invoice_id] = format_amount($open_invoice->invoice_balance); $invoice_payment_methods['invoice' . $open_invoice->invoice_id] = $open_invoice->payment_method; }Committable suggestion skipped: line range outside the PR's diff.
application/modules/quotes/controllers/Quotes.php-65-71 (1)
65-71: Potential N+1 query performance issue.The code executes a database query for each quote in the loop. Consider updating the model to include service data in the JOIN (which was already added to
default_joinin Mdl_quotes) instead of querying separately.Since the
default_joininMdl_quotesalready includes the services table (after fixing to use LEFT JOIN as noted in that file's review), theservice_nameshould already be available on each quote object. This entire loop may be unnecessary.🔎 Proposed fix
After fixing the JOIN in
Mdl_quotes.phpto useLEFT JOIN:$this->mdl_quotes->paginate(site_url('quotes/status/' . $status), $page); $quotes = $this->mdl_quotes->result(); - foreach ($quotes as $quote) { - $service = $this->db->query('SELECT service_name FROM ip_services WHERE service_id = ?', $quote->service_id)->result_array(); - if ($service && $service[0] && $service[0]['service_name']) - $quote->service_name = $service[0]['service_name']; - else - $quote->service_name = null; - } - - $services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); + // service_name is already available via the JOIN in default_join() + // Fetch all services for dropdown options + $this->load->model('services/mdl_services'); + $services = $this->mdl_services->get()->result_array();Committable suggestion skipped: line range outside the PR's diff.
application/modules/quotes/controllers/Quotes.php-154-161 (1)
154-161: Remove redundant service_name query.Similar to the status() method, the service_name should already be available via the model's default_join. This query is redundant and adds unnecessary database load.
🔎 Proposed fix
$quote = $this->mdl_quotes->get_by_id($quote_id); if ( ! $quote) { show_404(); } - $service = $this->db->query('SELECT service_name FROM ip_services WHERE service_id = ?', $quote->service_id)->result_array(); - - if ($service && $service[0] && $service[0]['service_name']) - $quote->service_name = $service[0]['service_name']; - else - $quote->service_name = null; - - $services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); - + // service_name is already available via the JOIN in default_join() + // Fetch all services for dropdown options + $this->load->model('services/mdl_services'); + $services = $this->mdl_services->get()->result_array();Committable suggestion skipped: line range outside the PR's diff.
application/modules/quotes/models/Mdl_quotes.php-216-216 (1)
216-216: Remove unused $services variable.The
$servicesvariable is queried but never used in thecopy_quotemethod. This was correctly flagged by PHPMD static analysis.🔎 Proposed fix
$quote_items = $this->mdl_quote_items->where('quote_id', $source_id)->get()->result(); - - $services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); foreach ($quote_items as $quote_item) {Based on static analysis.
🟡 Minor comments (3)
application/modules/quotes/views/partial_quote_table.php-46-54 (1)
46-54: Fix HTML entity syntax error.Line 50 uses
 without a semicolon, which will render as literal text (instead of a non-breaking space. The correct entity is .🔎 Proposed fix
<?php _htmlsc(format_client($quote)); if ($quote->service_name) { - echo ' ('; + echo ' ('; _htmlsc($quote->service_name); echo ')'; } ?>application/modules/invoices/views/partial_invoice_table.php-60-68 (1)
60-68: Fix HTML entity typo and improve formatting.Line 64 has a typo:
 (should be ((missing semicolon in the HTML entity).Additionally, the indentation uses tabs inconsistently (lines 60-68), while the rest of the file uses spaces.
🔎 Proposed fix
<td> <a href="<?php echo site_url('clients/view/' . $invoice->client_id); ?>" - title="<?php _trans('view_client'); ?>"> + title="<?php _trans('view_client'); ?>"> <?php - _htmlsc(format_client($invoice)); - if ($invoice->service_name) { - echo ' ('; - _htmlsc($invoice->service_name); - echo ')'; - } + _htmlsc(format_client($invoice)); + if ($invoice->service_name) { + echo ' ('; + _htmlsc($invoice->service_name); + echo ')'; + } ?> </a> </td>application/modules/services/controllers/Services.php-13-19 (1)
13-19: Fix copy-paste error in documentation comments.The documentation comments reference "Tax_Rates" but the actual class name is "Services".
🔎 Proposed fix
/** - * Class Tax_Rates + * Class Services */ class Services extends Admin_Controller { /** - * Tax_Rates constructor. + * Services constructor. */
🧹 Nitpick comments (13)
application/modules/filter/controllers/Ajax.php (2)
35-42: Avoid N+1 queries: Use JOIN in the model instead.This code executes a separate query for each invoice to fetch
service_name, resulting in N+1 queries. The model'sdefault_select()method should include a LEFT JOIN withip_servicesto fetchservice_namein a single query.🔎 Recommended approach
In
application/modules/invoices/models/Mdl_invoices.php, updatedefault_select()to include:$this->db->select('ip_services.service_name'); $this->db->join('ip_services', 'ip_services.service_id = ip_invoices.service_id', 'left');Then this enrichment loop can be removed entirely.
65-72: Avoid N+1 queries: Use JOIN in the model instead.This code executes a separate query for each quote to fetch
service_name, resulting in N+1 queries. The model'sdefault_select()method should include a LEFT JOIN withip_servicesto fetchservice_namein a single query.🔎 Recommended approach
In
application/modules/quotes/models/Mdl_quotes.php, updatedefault_select()to include:$this->db->select('ip_services.service_name'); $this->db->join('ip_services', 'ip_services.service_id = ip_quotes.service_id', 'left');Then this enrichment loop can be removed entirely.
application/views/invoice_templates/pdf/InvoicePlane.php (1)
56-62: Consider simplifying the conditional check.The output logic is correct and properly escaped. However, the condition can be simplified from
isset($invoice->service_name) && $invoice->service_nameto!empty($invoice->service_name).🔎 Proposed simplification
<b><?php _htmlsc(format_client($invoice)); ?></b> <?php - if (isset ($invoice->service_name) && $invoice->service_name) { + if (!empty($invoice->service_name)) { echo '<br>'; _htmlsc($invoice->service_name); } ?>application/modules/quotes/views/modal_create_quote.php (1)
17-18: Inconsistent indentation: mixed tabs and spaces.Lines 17-18 (and throughout the service_id block) use tabs for indentation, while the rest of the file uses spaces. Maintain consistent indentation throughout the file.
application/modules/layout/views/includes/navbar.php (1)
23-26: Navigation menu updated correctly for services feature.The two new menu items (
add_serviceandview_services) follow the existing pattern and integrate well with the services feature. The structure and approach are consistent with other menu sections.Minor note: Lines 24-26 use tabs for indentation while the rest of the file uses spaces, but this is a minor formatting inconsistency.
application/modules/quotes/views/view.php (1)
79-80: Inconsistent indentation in save payload.Lines 79-80 use tabs while surrounding code uses spaces. Maintain consistent indentation throughout the file.
application/views/quote_templates/pdf/InvoicePlane.php (1)
64-71: Service name correctly displayed in PDF template.The conditional rendering of
service_nameis implemented correctly, checking both that the property exists and has a non-empty value before displaying it in the PDF.Minor note: Lines 65-70 use tabs for indentation while the rest of the file uses spaces, but this is a minor formatting inconsistency.
application/modules/invoices/views/view.php (1)
426-426: Consider using a more semantic CSS class name.The service dropdown is wrapped in a
<div>withclass="client-address", which is semantically misleading since this section contains service information, not address information.🔎 Proposed fix
- <div class="client-address"> + <div class="client-service">Note: This change would require updating the CSS if
.client-addresshas specific styling that should be preserved.application/modules/quotes/models/Mdl_quotes.php (1)
546-561: Add validation in set_quote_service method.The
set_quote_servicemethod updates the quote's service_id but doesn't validate that the provided$service_idexists in the services table, which could lead to referential integrity issues.🔎 Proposed enhancement
public function set_quote_service($quote_id, $service_id) { $quote = $this->get_by_id($quote_id); if (!empty($quote)) { + // Validate service exists (allow 0 for "no service") + if ($service_id != 0) { + $this->load->model('services/mdl_services'); + $service = $this->mdl_services->where('service_id', $service_id)->get()->row(); + if (empty($service)) { + return false; // Invalid service_id + } + } + $this->db->where('quote_id', $quote_id); $this->db->set('service_id', $service_id); $this->db->update('ip_quotes'); + return true; } + return false; }application/modules/invoices/controllers/Ajax.php (2)
23-29: Formatting inconsistency with indentation.Lines 25-26 use tabs for indentation while the rest of the codebase appears to use different indentation. Consider maintaining consistent indentation throughout the file.
295-295: Refactor to use model method and eliminate code duplication.The same raw SQL query appears on lines 295 and 538. Consider using a dedicated model method (similar to
Mdl_clients::service_by_client()) instead of repeating raw SQL queries.🔎 Proposed refactor
Add a method to
Mdl_services:public function get_all_services() { return $this->db->query('SELECT service_id, service_name FROM ip_services ORDER BY service_name')->result_array(); }Then use it in both places:
-$services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); +$this->load->model('services/mdl_services'); +$services = $this->mdl_services->get_all_services();application/modules/services/controllers/Services.php (1)
109-117: Consider adding authorization checks.The delete method should verify that the user has permission to delete the service, and potentially check if the service is in use before deletion to prevent orphaned references.
application/modules/quotes/controllers/Ajax.php (1)
242-266: Refactor to eliminate code duplication.Line 253 contains the same raw SQL query that appears in the invoices controller. This should be refactored into a reusable model method to maintain DRY principles and improve maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
application/language/english/ip_lang.phpapplication/modules/clients/controllers/Clients.phpapplication/modules/clients/models/Mdl_clients.phpapplication/modules/filter/controllers/Ajax.phpapplication/modules/guest/controllers/Invoices.phpapplication/modules/invoices/controllers/Ajax.phpapplication/modules/invoices/controllers/Invoices.phpapplication/modules/invoices/models/Mdl_invoices.phpapplication/modules/invoices/views/modal_copy_invoice.phpapplication/modules/invoices/views/modal_create_invoice.phpapplication/modules/invoices/views/partial_invoice_table.phpapplication/modules/invoices/views/view.phpapplication/modules/layout/views/includes/navbar.phpapplication/modules/payments/controllers/Payments.phpapplication/modules/quotes/controllers/Ajax.phpapplication/modules/quotes/controllers/Quotes.phpapplication/modules/quotes/models/Mdl_quotes.phpapplication/modules/quotes/views/modal_create_quote.phpapplication/modules/quotes/views/modal_quote_to_invoice.phpapplication/modules/quotes/views/partial_quote_table.phpapplication/modules/quotes/views/view.phpapplication/modules/services/controllers/Services.phpapplication/modules/services/models/Mdl_services.phpapplication/modules/services/views/form.phpapplication/modules/services/views/index.phpapplication/modules/setup/sql/038_1.6.x.sqlapplication/views/invoice_templates/pdf/InvoicePlane.phpapplication/views/quote_templates/pdf/InvoicePlane.php
🧰 Additional context used
🧬 Code graph analysis (20)
application/modules/quotes/views/modal_create_quote.php (1)
application/helpers/echo_helper.php (1)
_trans(71-74)
application/modules/quotes/controllers/Quotes.php (3)
application/modules/layout/controllers/Layout.php (1)
set(48-59)application/modules/guest/controllers/Invoices.php (1)
status(38-80)application/modules/invoices/controllers/Invoices.php (1)
status(38-86)
application/modules/services/views/form.php (3)
application/helpers/echo_helper.php (1)
_trans(71-74)application/modules/layout/controllers/Layout.php (1)
load_view(73-80)application/core/MY_Model.php (1)
form_value(432-437)
application/modules/invoices/views/modal_copy_invoice.php (1)
application/helpers/echo_helper.php (1)
_trans(71-74)
application/modules/invoices/controllers/Invoices.php (6)
application/modules/layout/controllers/Layout.php (1)
set(48-59)application/modules/guest/controllers/Invoices.php (1)
status(38-80)application/modules/quotes/controllers/Quotes.php (1)
status(38-89)application/modules/guest/controllers/Quotes.php (1)
status(38-75)application/helpers/trans_helper.php (1)
trans(25-59)application/modules/invoices/models/Mdl_invoices.php (1)
statuses(28-52)
application/modules/clients/models/Mdl_clients.php (1)
application/core/MY_Model.php (1)
result_array(323-326)
application/modules/services/controllers/Services.php (2)
application/modules/invoices/models/Mdl_invoices.php (1)
db_array(373-414)application/modules/quotes/models/Mdl_quotes.php (1)
db_array(264-299)
application/modules/filter/controllers/Ajax.php (1)
application/core/MY_Model.php (3)
get(149-162)result(301-304)result_array(323-326)
application/views/quote_templates/pdf/InvoicePlane.php (1)
application/helpers/echo_helper.php (1)
_htmlsc(39-46)
application/modules/invoices/controllers/Ajax.php (2)
application/core/MY_Model.php (3)
set_id(451-454)result_array(323-326)result(301-304)application/modules/clients/models/Mdl_clients.php (1)
service_by_client(314-318)
application/views/invoice_templates/pdf/InvoicePlane.php (3)
application/helpers/echo_helper.php (1)
_htmlsc(39-46)application/helpers/client_helper.php (1)
format_client(20-43)application/modules/guest/controllers/View.php (1)
invoice(22-87)
application/modules/invoices/views/partial_invoice_table.php (2)
application/helpers/echo_helper.php (2)
_trans(71-74)_htmlsc(39-46)application/helpers/client_helper.php (1)
format_client(20-43)
application/modules/quotes/models/Mdl_quotes.php (2)
application/modules/clients/models/Mdl_clients.php (1)
db_array(208-217)application/modules/invoices/models/Mdl_invoices.php (1)
db_array(373-414)
application/modules/services/views/index.php (3)
application/helpers/echo_helper.php (3)
_trans(71-74)_htmlsc(39-46)_csrf_field(91-96)application/helpers/pager_helper.php (1)
pager(22-45)application/modules/layout/controllers/Layout.php (1)
load_view(73-80)
application/modules/invoices/models/Mdl_invoices.php (1)
application/modules/quotes/models/Mdl_quotes.php (1)
db_array(264-299)
application/modules/quotes/views/view.php (3)
application/helpers/trans_helper.php (1)
trans(25-59)application/helpers/echo_helper.php (1)
_trans(71-74)application/modules/guest/controllers/View.php (1)
quote(143-193)
application/modules/invoices/views/modal_create_invoice.php (1)
application/helpers/echo_helper.php (1)
_trans(71-74)
application/modules/quotes/views/partial_quote_table.php (2)
application/helpers/echo_helper.php (2)
_trans(71-74)_htmlsc(39-46)application/helpers/client_helper.php (1)
format_client(20-43)
application/modules/payments/controllers/Payments.php (1)
application/core/MY_Model.php (1)
result_array(323-326)
application/modules/layout/views/includes/navbar.php (1)
application/helpers/trans_helper.php (1)
trans(25-59)
🪛 PHPMD (2.15.0)
application/modules/quotes/models/Mdl_quotes.php
216-216: Avoid unused local variables such as '$services'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (18)
application/language/english/ip_lang.php (1)
17-17: LGTM!The new service-related language keys follow the existing naming conventions and are appropriately placed within the language file.
Also applies to: 100-102, 555-555, 715-715
application/modules/quotes/views/modal_quote_to_invoice.php (1)
16-16: LGTM!The service_id propagation from quote to invoice is correctly implemented with proper syntax and follows the existing pattern for other hidden fields in the modal.
Also applies to: 55-56
application/modules/invoices/controllers/Invoices.php (1)
70-70: Services data loaded correctly for the view.Line 70 fetches all services in a single query for populating the view's service dropdown, which is the appropriate approach. This query is executed once per page load rather than per invoice.
application/modules/quotes/views/view.php (1)
320-340: No changes required. The service_id handling is correctly implemented. The database schema forip_quotes.service_idusesDEFAULT 0(not NULL), which aligns with the form's default option value of"0". The backend save logic inset_quote_service()correctly stores the posted value without requiring special conversion. The field has no validation rules, appropriately marking it as optional.application/modules/services/views/form.php (1)
1-35: LGTM!The service form implementation follows the framework's conventions:
- CSRF protection is properly implemented
- Form field uses the model's
form_valuemethod with HTML escaping- Required validation is set on the service_name field
- The structure aligns with other form views in the codebase
application/modules/services/views/index.php (1)
1-65: LGTM!The services index view is well-implemented:
- Follows the framework's standard patterns for index views
- CSRF protection is correctly applied to the delete form
- Output is properly escaped using
_htmlsc()- JavaScript confirmation prevents accidental deletions
- Clean, maintainable code structure
application/modules/invoices/views/view.php (1)
437-437: The translation key 'select_service' is properly defined in application/language/english/ip_lang.php at line 555 with the value 'Select a service'. No action required.application/modules/invoices/controllers/Ajax.php (2)
31-35: LGTM!The service_id handling is correct, with proper XSS sanitization applied.
527-550: LGTM!The modal correctly loads the services model and passes service data to the view. The service_id is properly sanitized using
intval().application/modules/services/models/Mdl_services.php (2)
1-29: LGTM!The model structure follows CodeIgniter conventions correctly, with proper table mapping and default query behavior for pagination.
31-62: LGTM!The validation rules are well-structured, providing separate validation contexts for client-specific and general service operations.
application/modules/services/controllers/Services.php (2)
28-39: LGTM!The index method correctly implements pagination and service listing.
41-65: LGTM!The form method correctly handles service creation and editing with proper validation.
application/modules/quotes/controllers/Ajax.php (5)
21-34: LGTM!The service_id handling in the save method is correct, with proper XSS sanitization.
119-129: LGTM!The db_array construction correctly includes service_id with proper syntax.
352-365: LGTM!The modal correctly includes service_id in the data payload.
367-402: LGTM!The change_client method correctly handles service_id with proper XSS sanitization.
467-560: LGTM!The quote_to_invoice method correctly propagates service_id from the quote to the generated invoice items.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
application/modules/quotes/models/Mdl_quotes.php (2)
214-221: Remove unused services query.Line 216 fetches all services from the database but the result is never used. The
service_idpropagation on Line 221 correctly copies the service from source items without needing this query.🔎 Proposed fix
$quote_items = $this->mdl_quote_items->where('quote_id', $source_id)->get()->result(); - - $services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); foreach ($quote_items as $quote_item) {
554-569: Fix misleading documentation.The method comment references resetting "quote and due date to there former values" but the method only updates
service_id. Additionally, "there" should be "their".🔎 Proposed fix
/** - * Reset the quote and due date to there former values + * Update the service association for a quote * @param $quote_id * @param $service_id */
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
application/modules/guest/controllers/Invoices.phpapplication/modules/invoices/models/Mdl_invoices.phpapplication/modules/invoices/views/modal_copy_invoice.phpapplication/modules/invoices/views/modal_create_invoice.phpapplication/modules/quotes/models/Mdl_quotes.phpapplication/modules/quotes/views/modal_create_quote.phpapplication/modules/services/controllers/Services.phpapplication/modules/setup/sql/038_1.6.x.sql
🚧 Files skipped from review as they are similar to previous changes (5)
- application/modules/services/controllers/Services.php
- application/modules/quotes/views/modal_create_quote.php
- application/modules/setup/sql/038_1.6.x.sql
- application/modules/invoices/views/modal_copy_invoice.php
- application/modules/invoices/views/modal_create_invoice.php
🧰 Additional context used
🧬 Code graph analysis (3)
application/modules/invoices/models/Mdl_invoices.php (1)
application/modules/quotes/models/Mdl_quotes.php (1)
db_array(264-308)
application/modules/guest/controllers/Invoices.php (1)
application/core/MY_Model.php (3)
result(301-304)result_array(323-326)row(312-315)
application/modules/quotes/models/Mdl_quotes.php (1)
application/modules/invoices/models/Mdl_invoices.php (1)
db_array(373-423)
🪛 PHPMD (2.15.0)
application/modules/quotes/models/Mdl_quotes.php
216-216: Avoid unused local variables such as '$services'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (8)
application/modules/quotes/models/Mdl_quotes.php (3)
69-70: LGTM!The addition of
ip_services.*to the select clause is consistent with the LEFT JOIN added indefault_join(), allowing service data to be included in query results when available.
87-88: LGTM!The LEFT JOIN for
ip_servicescorrectly addresses the past review concern, ensuring quotes without an associated service are not excluded from query results.
104-108: LGTM!The validation rule for
service_idappropriately omits therequiredconstraint, making service assignment optional for quotes.application/modules/invoices/models/Mdl_invoices.php (5)
60-61: LGTM!The inclusion of
ip_services.*in the select aligns with the LEFT JOIN and ensures service data (includingservice_name) is available in query results without additional queries.
84-85: LGTM - LEFT JOIN correctly used for services.The LEFT JOIN for
ip_servicesensures invoices without an associated service are not excluded from query results, addressing backward compatibility with existing data.
102-106: LGTM!The
service_idvalidation entry correctly omits theruleskey, making the service selection optional for invoices.
380-394: LGTM - Service ID handling implemented correctly.The code properly validates and resolves
service_id:
- Defaults to 0 for backward compatibility
- Only assigns the actual service_id if the record exists in
ip_services- Gracefully handles missing or invalid service selections
This addresses the previous review concern about hardcoded service_id values.
243-261: Verify ifserviceskey in item db_array is intentional.The
servicesarray (line 261) is added to each copied item's$db_array. This appears to be metadata for view rendering rather than a database column. Verify thatmdl_items->save()either uses this for UI purposes or safely ignores unknown keys during insert.#!/bin/bash # Check how mdl_items->save() handles the db_array and whether 'services' key is used or ignored ast-grep --pattern $'class Mdl_Items { $$$ save($$$) { $$$ } $$$ }' # Also check for any filtering of db_array keys before insert rg -n "services" application/modules/invoices/models/Mdl_items.php
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@ThierryHFR thanks for the PR. |
nielsdrost7
left a comment
nielsdrost7
left a comment
There was a problem hiding this comment.
@ThierryHFR
Consider making an option in the settings, just like they did with "Projects".
if "option services" is true, use that code that you just showed in your pull-request. That way all the other users are not seeing the new feature until they turn on that option.
| $invoice->service_name = null; | ||
| } | ||
|
|
||
| $services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); |
There was a problem hiding this comment.
@ThierryHFR I see this query show up a couple of times in the Controllers.
- Why not make a function for it in the model and then use that function instead?
- I don't like selecting all services when it's not necessary. I think you're doing a "select all services" per invoice here
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive service management feature for InvoicePlane, allowing users to assign department-specific services to invoices and quotes. The implementation adds a new ip_services database table and integrates service selection throughout the quote and invoice workflows.
Key Changes
- Database schema additions for services management and service-to-document associations
- Complete CRUD operations for services with dedicated controller, model, and views
- Service selection integrated into invoice and quote creation, editing, copying, and conversion workflows
- Service names displayed in document lists, PDFs, and payment views
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| application/modules/setup/sql/038_1.6.x.sql | Creates ip_services table and adds service_id columns to invoices/quotes tables |
| application/modules/services/* | New services module with controller, model, and views for CRUD operations |
| application/modules/quotes/models/Mdl_quotes.php | Adds service_id validation, join, and handling logic to quotes model |
| application/modules/quotes/controllers/*.php | Integrates service selection into quote workflows and retrieves service data |
| application/modules/quotes/views/*.php | Adds service dropdowns to quote forms and displays service names in lists |
| application/modules/invoices/models/Mdl_invoices.php | Adds service_id validation, join, and handling logic to invoices model |
| application/modules/invoices/controllers/*.php | Integrates service selection into invoice workflows and retrieves service data |
| application/modules/invoices/views/*.php | Adds service dropdowns to invoice forms and displays service names in lists |
| application/modules/payments/controllers/Payments.php | Retrieves and displays service names in payment views |
| application/modules/guest/controllers/Invoices.php | Fetches service names for guest invoice views |
| application/modules/filter/controllers/Ajax.php | Includes service name retrieval in invoice/quote filters |
| application/modules/layout/views/includes/navbar.php | Adds navigation menu items for services |
| application/language/english/ip_lang.php | Adds translation keys for service-related UI elements |
| application/views/*/pdf/InvoicePlane.php | Displays service names in PDF templates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!$this->mdl_services->prep_form($id)) { | ||
| show_404(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation using tabs mixed with spaces. The closing brace should use consistent whitespace (spaces) to match the CodeIgniter framework coding style.
| $this->db->insert('ip_client_services', [ | ||
| 'client_id' => $this->input->post('client_id'), | ||
| 'service_id' => $id + ]); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation using tabs instead of spaces. These closing braces should use consistent whitespace to match the CodeIgniter framework coding style.
| if (!$this->mdl_services->prep_form($id)) { | ||
| show_404(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation using tabs instead of spaces. These closing braces should use consistent whitespace to match the CodeIgniter framework coding style.
| </label> | ||
| <select name="service_id" id="service_id" | ||
| class="form-control input-sm simple-select" data-minimum-results-for-search="Infinity"> | ||
| <option value="0" selected><?php _trans('select service'); ?></option> |
There was a problem hiding this comment.
Translation key 'select service' should be 'select_service' with an underscore to match the convention used in ip_lang.php (line 555).
| <option value="0" selected><?php _trans('select service'); ?></option> | |
| <option value="0" selected><?php _trans('select_service'); ?></option> |
| <?php if ($invoice->is_read_only == 1 && $invoice->invoice_status_id == 4) { | ||
| echo 'disabled="disabled"'; | ||
| } ?>> | ||
| <option value="0" selected><?php _trans('select service'); ?></option> |
There was a problem hiding this comment.
Translation key 'select service' should be 'select_service' with an underscore to match the convention used in ip_lang.php (line 555).
| <option value="0" selected><?php _trans('select service'); ?></option> | |
| <option value="0" selected><?php _trans('select_service'); ?></option> |
| <div class="input-group" style="width: 100%;"> | ||
| <select name="service_id" id="service_id" class="form-control" style="width: 100%;" | ||
| autofocus="autofocus"> | ||
| <option value="0" selected><?php _trans('select service'); ?></option> |
There was a problem hiding this comment.
Translation key 'select service' should be 'select_service' with an underscore to match the convention used in ip_lang.php (line 555).
| <option value="0" selected><?php _trans('select service'); ?></option> | |
| <option value="0" selected><?php _trans('select_service'); ?></option> |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div><b><?php _htmlsc($quote->user_name); ?></b></div> | ||
| <div><b><?php _htmlsc($quote->user_name); ?></b> | ||
| <?php | ||
| if (isset ($quote->service_name) && $quote->service_name) { |
There was a problem hiding this comment.
Remove the space between 'isset' and the opening parenthesis. The correct syntax is isset($quote->service_name).
| if (isset ($quote->service_name) && $quote->service_name) { | |
| if (isset($quote->service_name) && $quote->service_name) { |
| <b><?php _htmlsc(format_client($invoice)); ?></b> | ||
| <b><?php _htmlsc(format_client($invoice)); ?></b> | ||
| <?php | ||
| if (isset ($invoice->service_name) && $invoice->service_name) { |
There was a problem hiding this comment.
Remove the space between 'isset' and the opening parenthesis. The correct syntax is isset($invoice->service_name).
| if (isset ($invoice->service_name) && $invoice->service_name) { | |
| if (isset($invoice->service_name) && $invoice->service_name) { |
| public function validation_client_rules() | ||
| { | ||
| return array( | ||
| 'service_name' => array( | ||
| 'field' => 'service_name', | ||
| 'label' => trans('service_name'), | ||
| 'rules' => 'required' | ||
| ), | ||
| 'client_id' => array( | ||
| 'field' => 'client_id', | ||
| 'label' => trans('client_id'), | ||
| 'rules' => 'required' | ||
| ), | ||
| ); | ||
| } | ||
| /** | ||
| * @return array | ||
| */ |
There was a problem hiding this comment.
The validation_client_rules() method is defined but never used in the codebase. Consider removing it or documenting its intended purpose.
| public function validation_client_rules() | |
| { | |
| return array( | |
| 'service_name' => array( | |
| 'field' => 'service_name', | |
| 'label' => trans('service_name'), | |
| 'rules' => 'required' | |
| ), | |
| 'client_id' => array( | |
| 'field' => 'client_id', | |
| 'label' => trans('client_id'), | |
| 'rules' => 'required' | |
| ), | |
| ); | |
| } | |
| /** | |
| * @return array | |
| */ |
| $services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); | ||
|
|
||
| foreach ($quote_items as $quote_item) { | ||
| $db_array = [ | ||
| 'quote_id' => $target_id, | ||
| 'quote_id' => $target_id, | ||
| 'service_id' => $quote_item->service_id, |
There was a problem hiding this comment.
The service_id assignment is copying from quote_item, but quote items don't have a service_id field. Services are associated with quotes, not individual quote items. This should use $quote->service_id or be removed from item copying logic.
| $services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); | |
| foreach ($quote_items as $quote_item) { | |
| $db_array = [ | |
| 'quote_id' => $target_id, | |
| 'quote_id' => $target_id, | |
| 'service_id' => $quote_item->service_id, | |
| foreach ($quote_items as $quote_item) { | |
| $db_array = [ | |
| 'quote_id' => $target_id, |
| $quote_items = $this->mdl_quote_items->where('quote_id', $source_id)->get()->result(); | ||
| $quote_items = $this->mdl_quote_items->where('quote_id', $source_id)->get()->result(); | ||
|
|
||
| $services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); |
There was a problem hiding this comment.
The $services variable is retrieved but never used in the copy_quote method. Remove this unused query.
| $services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); |
| $db_array = [ | ||
| 'invoice_id' => $target_id, | ||
| 'invoice_id' => $target_id, | ||
| 'service_id' => $invoice_item->service_id, |
There was a problem hiding this comment.
The service_id assignment is copying from invoice_item, but invoice items don't have a service_id field. Services are associated with invoices, not individual items. This should use the invoice's service_id or be removed.
| // Copy the items | ||
| $invoice_items = $this->mdl_items->where('invoice_id', $source_id)->get()->result(); | ||
|
|
||
| $services = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name')->result_array(); |
There was a problem hiding this comment.
The $services variable is retrieved but never used in the copy_invoice method. Remove this unused query.
| 'item_product_unit' => $invoice_item->item_product_unit, | ||
| 'item_product_unit_id' => $invoice_item->item_product_unit_id, | ||
| 'item_product_unit_id' => $invoice_item->item_product_unit_id, | ||
| 'services' => $services, |
There was a problem hiding this comment.
Adding 'services' to the item db_array is incorrect. The $services array should not be saved as an item field. Remove this line.
| public function service_by_client () | ||
| { | ||
| $query = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name'); | ||
| return $query->result_array(); | ||
| } |
There was a problem hiding this comment.
The method name service_by_client is misleading as it doesn't filter by client but returns all services. Consider renaming to get_all_services() or similar to accurately reflect its behavior.
| public function service_by_client () | |
| { | |
| $query = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name'); | |
| return $query->result_array(); | |
| } | |
| public function get_all_services() | |
| { | |
| $query = $this->db->query('SELECT service_id, service_name FROM ip_services WHERE 1 ORDER BY service_name'); | |
| return $query->result_array(); | |
| } | |
| /** | |
| * @deprecated Use get_all_services() instead. | |
| * | |
| * Kept for backwards compatibility. This method returns all services | |
| * and does not filter by client. | |
| */ | |
| public function service_by_client() | |
| { | |
| return $this->get_all_services(); | |
| } |
| /* | ||
| // $invoices = $this->mdl_invoices->result(); | ||
| $invoices = $this->mdl_invoices->by_client($client_id)->limit(20)->get()->result(); | ||
| foreach ($invoices as $invoice) { | ||
| $service = $this->db->query('SELECT service_name FROM ip_services WHERE service_id = ?', $invoice->service_id)->result_array(); | ||
| if ($service && $service[0] && $service[0]['service_name']) | ||
| $invoice->service_name = $service[0]['service_name']; | ||
| else | ||
| $invoice->service_name = null; | ||
| } | ||
| $quotes = $this->mdl_quotes->by_client($client_id)->limit(20)->get()->result(); | ||
| foreach ($quotes as $quote) { | ||
| $service = $this->db->query('SELECT service_name FROM ip_services WHERE service_id = ?', $quote->service_id)->result_array(); | ||
| if ($service && $service[0] && $service[0]['service_name']) | ||
| $quote->service_name = $service[0]['service_name']; | ||
| else | ||
| $quote->service_name = null; | ||
| } | ||
| */ | ||
|
|
There was a problem hiding this comment.
Large block of commented-out code (lines 201-222) should be removed if not needed, or converted to proper implementation if it is intended functionality.
| /* | |
| // $invoices = $this->mdl_invoices->result(); | |
| $invoices = $this->mdl_invoices->by_client($client_id)->limit(20)->get()->result(); | |
| foreach ($invoices as $invoice) { | |
| $service = $this->db->query('SELECT service_name FROM ip_services WHERE service_id = ?', $invoice->service_id)->result_array(); | |
| if ($service && $service[0] && $service[0]['service_name']) | |
| $invoice->service_name = $service[0]['service_name']; | |
| else | |
| $invoice->service_name = null; | |
| } | |
| $quotes = $this->mdl_quotes->by_client($client_id)->limit(20)->get()->result(); | |
| foreach ($quotes as $quote) { | |
| $service = $this->db->query('SELECT service_name FROM ip_services WHERE service_id = ?', $quote->service_id)->result_array(); | |
| if ($service && $service[0] && $service[0]['service_name']) | |
| $quote->service_name = $service[0]['service_name']; | |
| else | |
| $quote->service_name = null; | |
| } | |
| */ |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
2 participants
Description
I've been using InvoicePlane for a number of years.
I have several customers who have departments, and these customers ask us to send invoices directly to the relevant department.
So I've added service management for customers.
I finally took the time to clean it up and offer it as a feature.
Related Issue
Motivation and Context
Screenshots (if appropriate):
Add 2 entries to add and list services
Service creation page
Service selection when creating a quote or invoice
When listing quotes or invoices, the service appears in brackets.
On quotations or invoices, the service appears under the customer's name.
Pull Request Checklist
Issue Type (Please check one or more)
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.