Skip to content
6 changes: 2 additions & 4 deletions src/client/app/redux/thunks/exportThunk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { createAppThunk } from './appThunk';
import { selectAnythingFetching } from '../../redux/selectors/apiSelectors';
import { RootState } from '../../store';
import { find, sortBy } from 'lodash';
import { estimateRawExportSizeMB } from '../../../../common/RawExportFileSize';

const selectCanExport = (state: RootState) => {
const fetchInProgress = selectAnythingFetching(state);
Expand Down Expand Up @@ -134,10 +135,7 @@ export const exportRawReadings = createAppThunk(
// the wrong value. The time to do this is small compared to most raw exports (if file is large
// when it matters).
const count = await dispatch(metersApi.endpoints.lineReadingsCount.initiate({ meterIDs, timeInterval })).unwrap();
// Estimated file size in MB. Note that changing the language effects the size about +/- 8%.
// This is just a decent estimate for larger files.
// This estimate is also present in src/server/routes/readings.js and must be kept consistent between files.
const fileSize = (count * 0.082 / 1000);
const fileSize = estimateRawExportSizeMB(count);
// Decides if the readings should be exported, true if should.
let shouldDownload = false;
if (fileSize <= adminState.defaultWarningFileSize) {
Expand Down
16 changes: 16 additions & 0 deletions src/common/RawExportFileSize.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public
* 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/.
*/

export const ESTIMATED_KB_PER_RAW_READING: number;
export const KB_PER_MB: number;
/**
* Estimates the size of a raw export in MB based on the number of readings.
* Note that changing the language effects the size about +/- 8%.
* This is just a decent estimate for larger files.
* @param {number} readingCount - The number of readings to estimate.
* @returns {number} The estimated size in MB.
*/
export function estimateRawExportSizeMB(readingCount: number): number;
24 changes: 24 additions & 0 deletions src/common/RawExportFileSize.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public
* 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/.
*/

const ESTIMATED_KB_PER_RAW_READING = 0.082;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two thoughts. First, given the estimate is in MB, would it be easier to make this const be in MB and avoid the KB_PER_MB conversion? I might make it 8.2e-5 so it is easier to understand. Second, I'm wondering why this is exported. I could not find uses outside this file except in the d.ts file but then that usage does not seem to be used anywhere else. I'm uncertain this const would ever be used anywhere else in OED and encapsulating it to the function may make sense. Thus, I was wondering about removing the export, placing the const inside the function and removing it from the t.ds file. It worked fine for me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the first point, I was keeping the structure of the formula the same as it originally was in the client but you're right that changing the estimate to be in MB makes a lot more sense now that its in a function. For the second point I was being safe just in case the values may be needed elsewhere but I realize now that encapsulating it for just the function is more reasonable.

const KB_PER_MB = 1000;
/**

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a blank line before the JSDco.

* Estimates the size of a raw export in MB based on the number of readings.
* Note that changing the language effects the size about +/- 8%.
* This is just a decent estimate for larger files.
* @param {number} readingCount - The number of readings to estimate.
* @returns {number} The estimated size in MB.
*/
function estimateRawExportSizeMB(readingCount) {
return readingCount * ESTIMATED_KB_PER_RAW_READING / KB_PER_MB;
}

module.exports = {
ESTIMATED_KB_PER_RAW_READING,
KB_PER_MB,
estimateRawExportSizeMB
};
5 changes: 2 additions & 3 deletions src/server/routes/readings.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const { isTokenAuthorized } = require('../util/userRoles');
const Preferences = require('../models/Preferences');
const User = require('../models/User');
const { success, failure } = require('./response');
const { estimateRawExportSizeMB } = require('../../common/RawExportFileSize');

const router = express.Router();

Expand Down Expand Up @@ -111,11 +112,9 @@ router.get('/line/raw/meter/:meter_id', optionalAuthMiddleware, async (req, res)
timeInterval = TimeInterval.fromString(req.query.timeInterval);
// Check if user is allowed to export.
let shouldDownload = false;
// Estimated file size. The full explanation of the estimate used can be found in the client.
// This estimate is also present in src/client/app/redux/thunks/exportThunk.ts and must be kept consistent between files.
// This count only checks a single meterID, while client testing checks multiple meterIDs, so the estimate is slightly different.
const count = await Reading.getCountByMeterIDAndDateRange(meterID, timeInterval.startTimestamp, timeInterval.endTimestamp, conn);
const fileSize = (count * 0.082 / 1000);
const fileSize = estimateRawExportSizeMB(count);
const preferences = await Preferences.get(conn);
if (fileSize <= preferences.defaultFileSizeLimit) {
// File size within limit, anyone can download.
Expand Down
30 changes: 30 additions & 0 deletions src/server/test/util/rawExportFileSizeTests.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public
* 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/.
*/

const chai = require('chai');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this works, many files get the chai, mocha & expect from common file to keep them consistent across files. I think that would be better. Having said that, OED probably should open an issue to do that across all tests to be consistent, assuming it causes no problem. Would you like to do that or should I? This should also get rid of the blank line between items as OED prefers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that. I was working from habit and forgot about OED’s common test file.. For the issue, I would be happy to open it after I take a closer look at the existing test files.


const expect = chai.expect;
const mocha = require('mocha');

const { estimateRawExportSizeMB } = require('../../../common/RawExportFileSize');

mocha.describe('Raw Export File Size Estimator', () => {
mocha.it('returns zero for zero readings', () => {
expect(estimateRawExportSizeMB(0)).to.be.closeTo(0, 0.001);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason you use 0.001 tolerance on these tests? In readings (see src/server/util/readingsUtils.js) it has a DELTA that is stricter. Stricter is better when it works. I would have thought this would be basic quasi-random numerical variation in the final digit given what is being tested. So, I tried 1e-15 and that worked fine. Maybe that should be used and set to a const in the file..

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice the delta value in readingsUtils.js. I now see that I gave the tests too much wiggle room. I will make the change

});

mocha.it('returns 0.082 MB for 1000 readings', () => {
expect(estimateRawExportSizeMB(1000)).to.be.closeTo(0.082, 0.001);
});

mocha.it('returns 0.11808 MB for 1440 readings', () => {
expect(estimateRawExportSizeMB(1440)).to.be.closeTo(0.11808, 0.001);
});

mocha.it('returns 0.71832 MB for 8760 readings', () => {
expect(estimateRawExportSizeMB(8760)).to.be.closeTo(0.71832, 0.001);
});
});
Loading