Max min values #1396

Rakesh-Ranga-Buram · 2024-11-27T13:12:09Z

Description

This code is related to the create and edit unit changes to incorporate the max and min value checks, and also the changes to make the max and min value associated with the unit to reflect when unit is selected on meter(both create and edit).

Partly Addresses #1307

(In general, OED likes to have at least one issue associated with each pull request. Replace [issue] with the OED GitHub issue number. In the preview you will see an issue description if you hover over that number. You can create one yourself before doing this pull request. This is where details are normally given on what is being addressed. Note you should not use the word "Fixes" if it does not completely address the issue since the issue would automatically be closed on merging the pull request. In that case use "Partly Addresses #[issue].)

Type of change

(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])

Note merging this changes the database configuration.
This change requires a documentation update

Checklist

(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)

I have followed the OED pull request ideas
I have removed text in ( ) from the issue request
You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

The work related to the trinary changes related to disable checks is still pending

create unit changes

edit unit changes

huss

Thanks to @lawsj, @mmehta2669 & @Rakesh-Ranga-Buram for working on this issue. Review and testing found it generally works but I have made several comments where issues where found. One comment that could not be easily placed into the code was:

src/server/util/compareUnits.js needs to have expectUnitToBeEquivalent updated to test min/max. src/server/test/db/unitTests.js & src/server/test/web/unitsTest.js need to be updated so it properly tests these values (and not just default values for all of them).

Please let me know if anything is not clear or you have thoughts.

src/client/app/components/unit/CreateUnitModalComponent.tsx

src/server/models/Unit.js

src/server/util/insertData.js

src/server/services/csvPipeline/uploadReadings.js

src/server/models/Unit.js

src/client/app/utils/input.ts

src/client/app/types/redux/units.ts

src/client/app/translations/data.ts

src/client/app/components/meters/EditMeterModalComponent.tsx

Disable changes

Requested Changes

Rakesh-Ranga-Buram · 2024-12-27T09:35:33Z

@huss, running tests on my local shows some errors and I guess the errors are related to the default_disable_checks on preferences. Should I go ahead and modify that column from boolean to the new enum created?

I think your next comment means this is resolved but let me know if it is not. It is correct that, in general, items that are an enum (or equivalent object in JS) should be matched with an enum in the database so they are equivalent. Also, the newer tests in src/server/test/db/enumTests.js to verify the enum in the database and JS are equivalent should be updated for any new enums added.

preference table changes

Rakesh-Ranga-Buram · 2024-12-28T02:51:24Z

@huss , I added the enum changes on preferences. This seems to be solving the issues I mentioned earlier.

Rakesh-Ranga-Buram · 2024-12-29T03:17:37Z

@huss, running tests on my local shows some errors and I guess the errors are related to the default_disable_checks on preferences. Should I go ahead and modify that column from boolean to the new enum created?

I think your next comment means this is resolved but let me know if it is not. It is correct that, in general, items that are an enum (or equivalent object in JS) should be matched with an enum in the database so they are equivalent. Also, the newer tests in src/server/test/db/enumTests.js to verify the enum in the database and JS are equivalent should be updated for any new enums added.

@huss , yes that is resolved. I will update src/server/test/db/enumTests.js and update this PR shortly

Adding DisableChecksType enum tests

Rakesh-Ranga-Buram · 2024-12-29T04:54:03Z

@huss ,now all the changes have been made

Pipeline changes

Rakesh-Ranga-Buram · 2025-01-13T06:36:11Z

@huss , the pipeline changes have been made, Can you please review this PR

huss · 2025-01-13T13:50:08Z

@huss , the pipeline changes have been made, Can you please review this PR

@Rakesh-Ranga-Buram I went to re-review this PR and noticed that two files have a merge conflict. Will you be resolving that? That will allow the CI to run for the tests.

resolving merge conflicts

Rakesh-Ranga-Buram · 2025-01-13T17:06:18Z

@huss , the pipeline changes have been made, Can you please review this PR

@Rakesh-Ranga-Buram I went to re-review this PR and noticed that two files have a merge conflict. Will you be resolving that? That will allow the CI to run for the tests.

@huss ,I have resolved the conflicts and all the tests are passing now

huss

Thanks to @Rakesh-Ranga-Buram for addressing previous comments. I've made more comments to consider. To keep the process cleaner, I resolved all the old comments and made new ones about the same area when needed. Also, the following comment from before does not seem to be addressed:

src/server/util/compareUnits.js needs to have expectUnitToBeEquivalent updated to test min/max. src/server/test/db/unitTests.js & src/server/test/web/unitsTest.js need to be updated so it properly tests these values (and not just default values for all of them).

Finally, I tested uploads to a modest extent. The reject bad seems to have an issue (see comment) so full testing is awaiting resolution.

Please let me know if anything is not clear, you have thoughts or need help.

huss · 2025-01-26T17:06:44Z

src/server/test/db/enumTests.js

@@ -19,65 +19,65 @@ const { mocha, expect } = require('../common');

 mocha.describe('Enums JS to DB', () => {


Tab indenting should be used. It was a mix before and I just noticed. Easy to do in VSC so let me know if you prefer that I do it.

huss · 2025-01-26T17:21:10Z

src/client/app/components/unit/CreateUnitModalComponent.tsx

+						<Row xs='1' lg='2'>
+							{/* minVal input */}
+							<Col><FormGroup>
+								<Label for='minVal'>{translate('min.val')}</Label>


Should be min.value.

huss · 2025-01-26T17:40:15Z

src/server/migrations/1.0.0-2.0.0/sql/units/alter_units_table_add_columns.sql

+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+ALTER TABLE units
+    ADD COLUMN IF NOT EXISTS min_val FLOAT NOT NULL DEFAULT -9007199254740991 CHECK (min_val::FLOAT >= -9007199254740991),


I'm unclear on the value of these checks as it seems to verify it is not out of bounds of allowed min/max float. Would it make sense to remove these checks and check that max_val >= min_val? This also applies to meters (and preferences but those seem likely to go away).

huss · 2025-01-26T21:04:41Z

src/server/migrations/1.0.0-2.0.0/index.js

@@ -20,5 +20,9 @@ module.exports = {
        // It should not matter but first rename cik and then do units.
        await db.none(sqlFile('../migrations/1.0.0-2.0.0/sql/cik/alter_cik_table.sql'));
        await db.none(sqlFile('../migrations/1.0.0-2.0.0/sql/units/alter_units_table.sql'));
+        await db.none(sqlFile('../migrations/1.0.0-2.0.0/sql/units/alter_units_table_add_columns.sql'));
+        await db.none(sqlFile('../migrations/1.0.0-2.0.0/sql/units/add_disable_checks_types.sql'));


It would be best to add the DB types for checks before the column using it is added in the line above.

huss · 2025-01-26T21:06:53Z

src/server/migrations/1.0.0-2.0.0/sql/units/alter_units_table_add_columns.sql

+ALTER TABLE units
+    ADD COLUMN IF NOT EXISTS min_val FLOAT NOT NULL DEFAULT -9007199254740991 CHECK (min_val::FLOAT >= -9007199254740991),
+    ADD COLUMN IF NOT EXISTS max_val FLOAT NOT NULL DEFAULT 9007199254740991 CHECK (max_val::FLOAT <= 9007199254740991),
+    ADD COLUMN IF NOT EXISTS disable_checks disable_checks_type NOT NULL DEFAULT 'reject_none'


I think the current default is reject_all and that is what is in the design document for meters so units should follow that. Applies to create_units_table.sql too. meters & preferences should do the same.

huss · 2025-01-30T16:31:00Z

src/server/sql/meter/create_meters_table.sql

@@ -70,5 +70,5 @@ CREATE TABLE IF NOT EXISTS meters (
    min_date TIMESTAMP NOT NULL DEFAULT '1970-01-01 00:00:00+00:00',
    max_date TIMESTAMP NOT NULL DEFAULT '6970-01-01 00:00:00+00:00',
    max_error INTEGER NOT NULL DEFAULT 75,
-    disable_checks BOOLEAN DEFAULT false
+    disable_checks disable_checks_type NOT NULL DEFAULT 'reject_none'


huss · 2025-01-30T16:31:52Z

src/server/sql/preferences/insert_default_row.sql

@@ -21,7 +21,7 @@ IF NOT EXISTS(SELECT *
 	default_meter_disable_checks, default_help_url) 
 	VALUES ('', 'line', FALSE, 'en', NULL, 5, 25, FALSE, 'meters', '00:15:00', 
 	-9007199254740991, 9007199254740991, '1970-01-01 00:00:00+00:00', '6970-01-01 00:00:00+00:00',
-	0, 75, FALSE, 'https://openenergydashboard.org/');
+	0, 75, 'reject_all', 'https://openenergydashboard.org/');


This may need to go for min/max/reject.

huss · 2025-01-30T16:34:12Z

src/server/sql/unit/create_units_table.sql

@@ -12,5 +12,8 @@ CREATE TABLE IF NOT EXISTS units (
 	suffix VARCHAR(50) DEFAULT '',
 	displayable displayable_type NOT NULL,
 	preferred_display BOOLEAN NOT NULL,
-	note TEXT
+	note TEXT,
+	min_val FLOAT NOT NULL DEFAULT -9007199254740991 CHECK (min_val::FLOAT >= -9007199254740991),


See the alter unit file for comments about this. Need to be consistent with any changes.

huss · 2025-01-30T16:35:39Z

src/server/sql/unit/insert_new_unit.sql

@@ -2,6 +2,6 @@
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

-INSERT INTO units(name, identifier, unit_represent, sec_in_rate, type_of_unit, suffix, displayable, preferred_display, note)
-VALUES (${name}, ${identifier}, ${unitRepresent}, ${secInRate}, ${typeOfUnit}, ${suffix}, ${displayable}, ${preferredDisplay}, ${note})
+INSERT INTO units(name, identifier, unit_represent, sec_in_rate, type_of_unit, suffix, displayable, preferred_display, note, max_val, min_val, disable_checks)


min comes before max in the table creation which makes sense to me. Here it is the other way. It would be nice if min came first.

huss · 2025-01-30T16:36:09Z

src/server/sql/unit/update_unit.sql

@@ -11,5 +11,8 @@ UPDATE units
 		suffix = ${suffix},
 		displayable = ${displayable},
 		preferred_display = ${preferredDisplay},
-		note = ${note}
+		note = ${note},
+		max_val = ${maxVal},


See insert unit file for min before max.

While reviewing I noticed this issue that is not part of the PR but thought it easiest to add to it.

mmehta2669 and others added 5 commits November 25, 2024 19:59

create unit changes

92afc55

edit unit changes

8953030

Merge pull request #32 from oss-slu/create_unit_1307

4edc46d

create unit changes

Merge pull request #33 from oss-slu/edit_unit_1307

c29b70c

edit unit changes

adding coulumns to the units table

a9380c9

huss requested changes Nov 28, 2024

View reviewed changes

Rakesh-Ranga-Buram and others added 4 commits December 4, 2024 20:52

Disable changes

e72f18e

Merge pull request #34 from oss-slu/Rakesh_disable_changes

fb0b593

Disable changes

Requested Changes

99f5de5

Merge pull request #35 from oss-slu/MinMax_RequestedChanges

55a44d8

Requested Changes

Rakesh-Ranga-Buram and others added 2 commits December 27, 2024 20:44

preference table changes

500c00e

Merge pull request #36 from oss-slu/MinMax_Preference

d162834

preference table changes

Rakesh-Ranga-Buram and others added 5 commits December 28, 2024 21:30

Merge branch 'development' into MaxMinValues

1b3a182

Update PreferencesComponent.tsx

b4f9431

Update data.ts

ca4f316

Adding DisableChecksType enum tests

788a59e

Merge pull request #37 from oss-slu/MinMax_EnumTests

057b53f

Adding DisableChecksType enum tests

Rakesh-Ranga-Buram marked this pull request as ready for review January 3, 2025 08:11

Rakesh-Ranga-Buram and others added 2 commits January 10, 2025 06:08

Pipeline changes

00870ec

Merge pull request #38 from oss-slu/MinMaxValues_pipeline

1851023

Pipeline changes

Rakesh-Ranga-Buram and others added 3 commits January 13, 2025 09:41

Merge branch 'development' into MaxMinValues

8523554

resolving merge conflicts

996bb4d

Merge pull request #42 from oss-slu/MinMaxValues_conflicts

587ab18

resolving merge conflicts

huss requested changes Jan 30, 2025

View reviewed changes

unrelated translation fix

5ae6979

While reviewing I noticed this issue that is not part of the PR but thought it easiest to add to it.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Max min values #1396

Max min values #1396

Rakesh-Ranga-Buram commented Nov 27, 2024

huss left a comment

Rakesh-Ranga-Buram commented Dec 27, 2024 •

edited by huss

Loading

Rakesh-Ranga-Buram commented Dec 28, 2024

Rakesh-Ranga-Buram commented Dec 29, 2024

Rakesh-Ranga-Buram commented Dec 29, 2024

Rakesh-Ranga-Buram commented Jan 13, 2025

huss commented Jan 13, 2025

Rakesh-Ranga-Buram commented Jan 13, 2025

huss left a comment

huss Jan 26, 2025

huss Jan 26, 2025

huss Jan 26, 2025

huss Jan 26, 2025

huss Jan 26, 2025

huss Jan 30, 2025

huss Jan 30, 2025

huss Jan 30, 2025

huss Jan 30, 2025

huss Jan 30, 2025

		@@ -19,65 +19,65 @@ const { mocha, expect } = require('../common');

		mocha.describe('Enums JS to DB', () => {

Max min values #1396

Are you sure you want to change the base?

Max min values #1396

Conversation

Rakesh-Ranga-Buram commented Nov 27, 2024

Description

Type of change

Checklist

Limitations

huss left a comment

Choose a reason for hiding this comment

Rakesh-Ranga-Buram commented Dec 27, 2024 • edited by huss Loading

Rakesh-Ranga-Buram commented Dec 28, 2024

Rakesh-Ranga-Buram commented Dec 29, 2024

Rakesh-Ranga-Buram commented Dec 29, 2024

Rakesh-Ranga-Buram commented Jan 13, 2025

huss commented Jan 13, 2025

Rakesh-Ranga-Buram commented Jan 13, 2025

huss 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

Rakesh-Ranga-Buram commented Dec 27, 2024 •

edited by huss

Loading