Usage report functionality from main branch #485

ajit3190 · 2025-01-16T12:19:25Z

No description provided.

jtoliver-quoin · 2025-01-22T16:18:50Z

app/javascript/components/form/components/form-section-field.jsx

@@ -52,6 +52,7 @@ function FormSectionField({ checkErrors, field, formMethods, formMode, disableUn
        <div className={classes} data-testid="form-section-field">
          {renderField}
        </div>
+        <div className={`${classes} ${css[commonInputProps.id]}`}>{renderField}</div>


What is this for? We are already rendering renderField above

jtoliver-quoin · 2025-01-22T16:19:51Z

app/javascript/components/form/components/styles.css

+
+.To{
+  margin-left: 307px;
+    margin-top: -103px;


Correct indents, and rename this css class to something a little more verbose if still used. If not used delete it.

jtoliver-quoin · 2025-01-22T16:20:57Z

app/javascript/components/form/fields/date-picker.jsx

@@ -0,0 +1,74 @@
+// Copyright (c) 2014 - 2023 UNICEF. All rights reserved.


npm run test and npm run lint and fix test and lint issues from output

jtoliver-quoin · 2025-01-22T16:24:58Z

app/javascript/components/form/fields/date-picker.jsx

+import PropTypes from "prop-types";
+import DateFnsUtils from "@date-io/date-fns";
+import { Controller, useWatch } from "react-hook-form";
+import { MuiPickersUtilsProvider, KeyboardDatePicker } from "@material-ui/pickers";


Remove this file and use the DateInput component in this directory

jtoliver-quoin · 2025-01-22T16:26:02Z

app/javascript/components/form/use-form-field.js

@@ -232,6 +234,8 @@ export default (field, { checkErrors, errors, formMode, disableUnderline }) => {
        return HiddenInput;
      case LINK_FIELD:
        return LinkField;
+      case DATE_PICKER:


Remove this

jtoliver-quoin · 2025-01-22T16:42:53Z

app/javascript/components/pages/admin/usage-reports/container.jsx

+          close={dialogClose}
+          setPending={setDialogPending}
+        />
+


Remove space

jtoliver-quoin · 2025-01-22T16:46:25Z

app/javascript/components/pages/admin/usage-reports/container.jsx

@@ -0,0 +1,41 @@
+// Copyright (c) 2014 - 2023 UNICEF. All rights reserved.
+
+import { SwapVert } from "@material-ui/icons";


Change package @mui/icons-material

jtoliver-quoin · 2025-01-22T16:49:13Z

app/javascript/components/pages/admin/usage-reports/export/component.jsx

+import { saveExport } from "../../../../../components/record-actions/exports/action-creators";
+import { formatFileName } from "../../../../../components/record-actions/exports/utils";
+
+


Remove one empty line

jtoliver-quoin · 2025-01-22T16:54:31Z

app/javascript/components/pages/admin/usage-reports/export/component.jsx

+  // Form submission
+  const onSubmit = getData => {
+    // Check if any of the required fields are null
+    if (!getData.From) {


Use yup for validation instead of the following. You can pass a schema into like

import { object, string } from "yup"; export const validationSchema = object().shape({ message: string().required() }); <Form validations={validationSchema} />

into the form component

https://github.com/jquense/yup

jtoliver-quoin · 2025-01-22T16:56:44Z

app/javascript/components/form/component.jsx

@@ -35,7 +35,8 @@ function Component({
  formClassName,


The changes in this file aren't needed.

pnabutovsky

Make sure all unit tests and linters pass for both JS and Ruby

pnabutovsky · 2025-01-21T18:20:29Z

app/controllers/api/v2/usage_reports_controller.rb

+# API endpoint for generating exports, in bulk or for individual records
+class Api::V2::UsageReportsController < ApplicationApiController
+  def create
+    @export = ExportService.build(export_params, current_user)


This endpoint needs an authorization check like the others. Otherwise stats can be pulled by anyone who hasn't logged in.

pnabutovsky · 2025-01-21T18:21:35Z

app/controllers/api/v2/usage_reports_controller.rb

+    @export_params ||= params.require(:data).permit(
+      :record_type, :export_format,
+      :order, :query, :file_name, :password, :selected_from_date, :selected_to_date,
+      { custom_export_params: {} }, { filters: {} }, :match_criteria


If we know exactly what the filters and custom export params are let's explicitly specify them in this structure rather than just using the permissive {}

pnabutovsky · 2025-01-21T18:23:58Z

app/controllers/api/v2/users_controller.rb

@@ -5,6 +5,7 @@
 # Users CRUD API
 class Api::V2::UsersController < ApplicationApiController
  include Api::V2::Concerns::Pagination
+  include Api::V2::Concerns::Export


Does this code actually get used?

pnabutovsky · 2025-01-21T18:31:17Z

app/models/bulk_export.rb

-  def export(password)
-    process_records_in_batches(500) { |records_batch| exporter.export(records_batch) }
+  def export(password, kpi_parameters)
+    if check_usage_report


I m not convinced you need to use the BulkExport here at all. The class is designed for exporting 1000's of records. A Primero instance should have significantly less users.

However, if you do need to leverage the bulk/batch logic of a bulk export (I don't see that being done in the proposed code) this would be a good situation to use class inheritance. Instead of inserting if/else logic in the BulkExport you should create a new UserKPIBulkExport that inherits from it. But again, I don't think you need to touch BulkExports at all.

pnabutovsky · 2025-01-21T18:33:32Z

app/models/exporters/usage_report_exporter.rb

+
+# Export forms to an Excel file (.xlsx)
+# rubocop:disable Metrics/ClassLength
+class Exporters::UsageReportExporter < Exporters::BaseExporter


This class has a lot in common with Exporters::ExcelExporter. You should inherit from that class instead of Exporters::BaseExporter

pnabutovsky · 2025-01-22T18:13:18Z

app/models/usage_report.rb

+
+# class for Usage Report
+class UsageReport
+  include ActiveModel::Model


Is there a reason that this is a Rails ActiveModel object rather than a plain Ruby object? Please move this business logic to a service, something like UsageReportService

pnabutovsky · 2025-01-22T18:15:21Z

app/models/exporters/usage_report_exporter.rb

+  end
+
+  def get_common_keys(module_id, start_date, end_date)
+    ['', UsageReport.total_records(module_id, Child).count, UsageReport.open_cases(module_id).count,


This is the wrong place to do the .count query. It needs to take place on the UsageReport

pnabutovsky · 2025-01-22T18:16:31Z

app/models/usage_report.rb

+class UsageReport
+  include ActiveModel::Model
+
+  class << self


Since the point of this is to run the .count queries, move the counts to this class

pnabutovsky · 2025-01-22T18:19:54Z

app/models/usage_report.rb

+
+    def new_records_quarter(module_id, start_date, end_date, recordtype)
+      recordtype.where(
+        "data->>'module_id' = ? AND CAST(data->>'created_at' AS DATE) BETWEEN ? AND ?",


The CAST(...) AS DATE may lead to performance issues in large databases. This query can be improved in v2.13 where certain JSON data fields are normalized into tables.

pnabutovsky · 2025-01-22T18:26:57Z

app/models/usage_report.rb

+      )
+    end
+
+    def total_services(module_id)


For the two methods below you are loading the entire set of cases into memory. This will crash Primero if it has more than 500-1000 cases. You need to do a more nuanced count query directly on the database that counts the services/followup subform entries. To do that you will probably need to do a CROSS JOIN. See how this was done for ReportableService on the Report. If you are writing complex SQL, don't forget to sanitize code to avoid SQL injections

pnabutovsky · 2025-01-22T18:31:37Z

config/routes.rb

@@ -124,6 +125,7 @@
        end
      end
      resources :bulk_exports, as: :exports, path: :exports, only: %i[index show create destroy]
+      resources :usage_reports


Restrict to only: %i[create]

afrahtrigyn added 6 commits January 16, 2025 17:47

Usage report functionality from main branch

8dcbe0b

Rubocop issues fxd

5c59895

Rubocop issues fxd2

0c85ffb

Rubocop issued fxd 3

080bc55

Rubocop issued fxd 3

3d9eb3b

Rubocop issued fxd 3

ee207d6

jtoliver-quoin requested changes Jan 22, 2025

View reviewed changes

pnabutovsky reviewed Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Usage report functionality from main branch #485

Usage report functionality from main branch #485

ajit3190 commented Jan 16, 2025

jtoliver-quoin Jan 22, 2025

jtoliver-quoin Jan 22, 2025

jtoliver-quoin Jan 22, 2025

jtoliver-quoin Jan 22, 2025

jtoliver-quoin Jan 22, 2025

jtoliver-quoin Jan 22, 2025

jtoliver-quoin Jan 22, 2025

jtoliver-quoin Jan 22, 2025

jtoliver-quoin Jan 22, 2025

jtoliver-quoin Jan 22, 2025

pnabutovsky left a comment

pnabutovsky Jan 21, 2025

pnabutovsky Jan 21, 2025

pnabutovsky Jan 21, 2025

pnabutovsky Jan 21, 2025

pnabutovsky Jan 21, 2025

pnabutovsky Jan 22, 2025

pnabutovsky Jan 22, 2025

pnabutovsky Jan 22, 2025

pnabutovsky Jan 22, 2025

pnabutovsky Jan 22, 2025

pnabutovsky Jan 22, 2025

		@@ -0,0 +1,74 @@
		// Copyright (c) 2014 - 2023 UNICEF. All rights reserved.

		@@ -0,0 +1,41 @@
		// Copyright (c) 2014 - 2023 UNICEF. All rights reserved.

		import { SwapVert } from "@material-ui/icons";

		import { saveExport } from "../../../../../components/record-actions/exports/action-creators";
		import { formatFileName } from "../../../../../components/record-actions/exports/utils";

Usage report functionality from main branch #485

Are you sure you want to change the base?

Usage report functionality from main branch #485

Conversation

ajit3190 commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnabutovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment