fix: enhanced CSV readings upload for new meter values by johnny603 · Pull Request #1613 · OpenEnergyDashboard/OED

johnny603 · 2026-04-22T19:51:06Z

Description

This PR adds support for overriding meter-based CSV upload parameters (timeZone, minVal, maxVal, minDate, maxDate, maxError, disableChecks) directly from the readings upload UI and ensures these values are respected throughout the backend pipeline.

Previously, the CSV upload pipeline always relied on meter values for validation and condition checks. This change updates the system so that:

Values provided in the request (req.body) are used as the source of truth
Meter values are only used as a fallback when parameters are missing (e.g., direct/script uploads)

Fixes #1519

Status: WIP

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

Validation behavior for empty strings vs. undefined values is inconsistent across the existing codebase and was not modified in this PR.
This may be addressed in a future issue.
Automated UI testing is not yet available in OED, so frontend behavior was verified manually.
Documentation updates for the new parameters are out of scope for this PR and can be handled in a future effort.

…t fallback add readings upload UI fields for timeZone, minVal, maxVal, minDate, maxDate, maxError, disableChecks sync new fields from selected meter and include them in unsaved-changes tracking extend readings upload param validation schema/defaults for the 7 override fields implement request-first resolution in uploadReadings (undefined/empty fallback to meter) apply resolved values to conditionSet and pass timezone override through pipeline processing harden FormData serialization to skip null/undefined values

huss

@johnny603 I want to thank you for working on this issue. I have reviewed the code and left some comments to consider. Please let me know if anything is not clear, you have thoughts/questions or you want anything from me.

huss · 2026-04-23T19:38:24Z

+ * @param {string} timeZone optional timezone override provided by the upload request.
 * @returns {object[]} {whether readings were all process (true) or false, all the messages from processing the readings as a string}
 */
 async function loadArrayInput(dataRows, meterID, mapRowToModel, timeSort, readingRepetition, isCumulative,


I am noting for the record that the new parameter and sometimes previous ones are not explicitly given when this function is called. It works fine but at some point OED should become consistent about this as it varies by call. This is not for this PR.

huss · 2026-04-23T20:38:38Z

 	};

-	const handleFileChange = (file: File) => {
+	const handleFileChange = (file: File | null) => {


Is this directly related to the changes you are making or something you noticed since it checks if file is okay on the next line?

huss · 2026-04-23T20:51:11Z

 			|| readingsData.update !== ReadingsCSVUploadDefaults.update
-			|| readingsData.useMeterZone !== readingsData.useMeterZone
-			|| readingsData.warnOnCumulativeReset !== readingsData.warnOnCumulativeReset
+			|| readingsData.useMeterZone !== ReadingsCSVUploadDefaults.useMeterZone


I just want to verify that you fixed a bug here and on the next line where the two items compared were the same.

huss · 2026-04-23T20:52:01Z

 			lengthVariation: selectedMeter.readingVariation,
 			endOnly: selectedMeter.endOnlyTime,
 			timeSort: MeterTimeSortType[selectedMeter.timeSort as keyof typeof MeterTimeSortType],
+			timeZone: selectedMeter.timeZone ?? '',


I'm just thinking about the need for ?? and when it is null. Insight?

huss · 2026-04-23T20:53:21Z

+													{translate('meter.time.zone')}
+												</div>
+											</Label>
+											<TimeZoneSelect current={readingsData.timeZone ?? null} handleClick={timeZone => handleTimeZoneChange(timeZone)} />


Asking about the null here and not used with some other TimeZoneSelect uses. I think all these null/''/undefined are probably related.

…ngs overrides

huss

@johnny603 Thank you for the updated code. Running the code has found no issues so far. I did made a couple of new comments. There are also several older comments that have yet to be addressed or commented on. I left these unresolved but resolved the others. I'm going to ask you not to resolve any comments but put in a reply about your thoughts or if it is done (or a thumbs up is fine too if done). This makes it easier for me to track what I need to look at again. Please let me know if you have any questions/thoughts, would like help or anything is not clear.

huss · 2026-04-26T18:41:32Z

 async function loadArrayInput(dataRows, meterID, mapRowToModel, timeSort, readingRepetition, isCumulative,
 	cumulativeReset, cumulativeResetStart, cumulativeResetEnd, readingGap, readingLengthVariation, isEndOnly,
-	shouldUpdate, conditionSet, conn, honorDst = false, relaxedParsing = false, useMeterZone = false, warnOnCumulativeReset = false) {
+	shouldUpdate, conditionSet, conn, honorDst = false, relaxedParsing = false, useMeterZone = 


This is really small but the use of useMeterZone is split across two lines. Could the false, be put on this line.

huss · 2026-04-26T19:11:58Z

 										</FormGroup>
 									</Col>
 								</Row>
+								<Row xs='1' lg='2'>


Now that I can run the code, I see the order and which column items are in does not match the meter page. I think the issue asked for them to be the same.

…pload Reorder frontend fields to match meter page; fix JSX and restore missing imports/handlers. Move new validation fields to end of schema; deduplicate disableChecks enum and standardize JSDoc for timezone. Add FormData null/undefined guard and fix import path in csvUploadDefaults. Add follow-up comment in loadArrayInput.js recommending options-object refactor for trailing args.

johnny603 · 2026-04-27T21:23:50Z

Quick follow-up, thanks for the review! A few items look unresolved from my side and I wanted to confirm how you'd like them handled (PR vs follow-up) since they produce no errors, also I also wanted to ask how did you test? How can we go about the testing phase?

huss · 2026-04-27T23:54:17Z

A few items look unresolved from my side and I wanted to confirm how you'd like them handled (PR vs follow-up) since they produce no errors, also I also wanted to ask how did you test? How can we go about the testing phase?

@johnny603 Thanks for asking. I just looked at the comments quickly and all seemed to have thumbs up which normally means they were addressed. Did I miss something or misinterpret the emoji? Once I get a handle on what is open/unresolved I can respond better. If it is enhancement then you can, to a great extent, decide to address or have an issue opened (by you or OED). The exception would be if OED thinks that it must be addressed before the PR can be merged. Let me know your thoughts and any questions.

johnny603 · 2026-04-28T03:25:36Z

A few items look unresolved from my side and I wanted to confirm how you'd like them handled (PR vs follow-up) since they produce no errors, also I also wanted to ask how did you test? How can we go about the testing phase?

@johnny603 Thanks for asking. I just looked at the comments quickly and all seemed to have thumbs up which normally means they were addressed. Did I miss something or misinterpret the emoji? Once I get a handle on what is open/unresolved I can respond better. If it is enhancement then you can, to a great extent, decide to address or have an issue opened (by you or OED). The exception would be if OED thinks that it must be addressed before the PR can be merged. Let me know your thoughts and any questions.

So I decided to mark future unresolved changes with a thumbs up to make it easier to review.

A couple of quick questions/thoughts as I review:

Would you prefer that any potential enhancements be handled within this PR, or split into separate issues for future work?
Are there any specific areas in the PR you think warrant a closer second look before merge (e.g., edge cases, performance, or structure of the CSV pipeline)?
If something is borderline between “nice-to-have” and “required,” is there a general guideline you follow for deciding whether it should block the merge?

Thanks again for the guidance—this helps me better understand how to proceed. Please let me know if there’s anything specific you’d like me to prioritize.

Best,
Johnny

huss · 2026-04-28T12:27:37Z

also I also wanted to ask how did you test? How can we go about the testing phase?

I used the admin meter page to create a meter where I changed the default values on the the new items so I could clearly see if they were used and not defaults. I then went to the CSV Readings page and selected that meter to verify all the right values were populated in the form. I then add a reading file and tried to upload to see what would happen. This can be repeated by changing the values on the CSV page to see if they are used. For example, changing the max to a value less than one of the readings should cause it to be rejected with an appropriate warning/error sent back and seen in the popup. Note you can copy the popup with multiple clicks on the text, put into a file and view as a web page to see it formatted nicely. I also looked at the database values directly (see https://openenergydashboard.org/developer/devDetails/#dbAccess) - this is only viable if you know some SQL. One can also remove any uploaded values via the DB if you want to be able to add again rather than create a new meter. I've attached a basic readings file that can be used and edited as desired. The first column is the readings, then the start date and finally the end date. There is also a help page for admins on uploading data from a CSV file but it is for the previous version of OED. I hope this helps and let me know if you need anything.
readings.csv

huss · 2026-04-28T12:28:48Z

Thanks again for the guidance—this helps me better understand how to proceed. Please let me know if there’s anything specific you’d like me to prioritize.

I plan to look at the updated PR in the near future. Once I do that I should be able to give input on all of this.

… healthy state

johnny603 · 2026-05-01T22:41:40Z

Hi, quick note: I fixed the local dev setup so oed-web-1 boots healthy under Docker Compose. These changes are strictly development-environment fixes and are not part of the CSV readings feature in this PR. I'm posting here to make it easier to stay working on this branch while keeping the PR focused on feature work.

Summary of what I changed and why

docker-compose.yml
Set OED_DB_HOST=database so the web container connects to the DB service via the compose network (was resolving to localhost in the web container, causing ECONNREFUSED).
Removed the obsolete version field to avoid compose warnings.
installOED.sh
Updated .env loading to avoid overwriting runtime environment variables provided by Docker Compose. This prevents .env from clobbering OED_DB_HOST=database (previously .env set localhost and web failed to reach the DB).
log.js
Broke a fragile circular dependency between logging and the DB by deferring DB model requires and lazily obtaining the DB connection. This makes startup resilient when the DB is not immediately available.
db.js
Fixed a bug in swapConnection() where the code was replacing the connmanager object instead of setting connmanager.connection. This ensures connection swapping behaves correctly.
Minor validation/sanity checks
Ran module-load checks, bash -n on the installer, docker compose config, and verified docker compose up now gets to a healthy oed-web-1.

Why I did this here
The changes were needed to make local development reliable (the original error was repeated ECONNREFUSED 127.0.0.1:5432 during install).
None of these edits modify application CSV-processing logic — they only restore a stable dev environment so we can test the CSV changes safely on this branch.

What I validated
Server modules load without the earlier getDB error.
Installer script syntax is valid.
Docker Compose config is valid.
docker compose up reaches a healthy oed-web-1 and the test user test / password is created so you can sign in and test the CSV upload flow.

Files changed (dev-only)
docker-compose.yml
installOED.sh
log.js
db.js

If you prefer, I can extract these dev-only patches into a separate branch/commit (or open a small follow-up PR labeled chore/dev-setup) to keep feature PR history tidy, let me know which you'd prefer.

Refers to issue #1617

huss · 2026-05-08T18:59:10Z

@johnny603 Thank you for the comment and code updates. I'm sorry for the slow reply. Unfortunately, per a recent Discord msg, I won't be able to do a complete review for a number of days. I'm sorry about this.

huss · 2026-05-27T14:53:07Z

@johnny603 I'm now able to work again and looking at this PR. I have not yet looked at the actual changes but wanted to start the conversation.

First, I wanted to make sure you are still available to work on OED and respond to comments.

if you prefer, I can extract these dev-only patches into a separate branch/commit (or open a small follow-up PR labeled chore/dev-setup) to keep feature PR history tidy, let me know which you'd prefer.

Yes, I think it would be better to extract these into a separate piece of work. Is PR #1610 this work or something else? My quick look indicated that it is different but does have the removal of the docker version. What is the status of this work as you opened that PR too. Note that PR #1554 that may be close tomerging may overlap this work. It removes the version, changes to newer syntax for the docker files, adds a second docker file to make it more secure, etc.

Refers to issue #1617

Did you mean PR #1617?

johnny603 added 2 commits April 22, 2026 12:44

added interface types with meter values, also updated default values

3d06acc

johnny603 marked this pull request as ready for review April 22, 2026 21:26

huss requested changes Apr 23, 2026

View reviewed changes

chore(pr-1613):address maintainer review feedback regarding CSV readi…

14dbf2e

…ngs overrides

johnny603 requested a review from huss April 24, 2026 05:34

huss requested changes Apr 26, 2026

View reviewed changes

johnny603 requested a review from huss April 27, 2026 21:24

johnny603 added 2 commits May 1, 2026 15:30

chore(dev-setup): stabilize local docker startup so oed-web-1 reaches…

25b5393

… healthy state

Merge branch 'development' into fix/csvReadings/1519

c33dc57

Conversation

johnny603 commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist

Limitations

Uh oh!

huss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huss Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huss Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

huss Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

huss Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

huss Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huss left a comment

Choose a reason for hiding this comment

Uh oh!

huss Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

huss Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

johnny603 commented Apr 27, 2026

Uh oh!

huss commented Apr 27, 2026

Uh oh!

johnny603 commented Apr 28, 2026

Uh oh!

huss commented Apr 28, 2026

Uh oh!

huss commented Apr 28, 2026

Uh oh!

johnny603 commented May 1, 2026

Uh oh!

huss commented May 8, 2026

Uh oh!

huss commented May 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

johnny603 commented Apr 22, 2026 •

edited

Loading