Export group #83

kooolbreez1 · 2017-04-13T00:57:08Z

This adds a button to the UI that when clicked exports the compressed data from the displayed graph for the currently selected meters as a CSV file.

…le to return the name of the selection (as seen in the select meters window) and what i believe to be meter data of some kind (still need to confirm that this is meter data and not some abstraction)

…ntainer to enable the export of the state object

# Conflicts: # src/client/app/components/LineChartComponent.jsx # src/client/app/containers/LineChartContainer.js

# Conflicts: # src/client/app/actions/readings.js # src/client/app/components/LineChartComponent.jsx # src/client/app/containers/LineChartContainer.js

# Conflicts: # src/client/app/actions/readings.js

…mpting the user for a name. linted

sandeepacharya464

I do not think reading.js or readMetasysData.js should have been changed. Am I missing something?

kooolbreez1 · 2017-04-13T01:33:40Z

I linted the file and deleted a blank space, I also accidentally changed a comment in dashboardContainer which has now been fixed.

RagingOcelot

Looks good, just a few small clean-up things

RagingOcelot · 2017-04-15T20:45:45Z

src/client/app/actions/exportData.js

+		data.forEach(reading => {
+			const info = reading.y;
+			const timeStamp = moment(reading.x).format('dddd MMM DD YYYY hh:mm a');
+			if (isStart) {


What is this conditional for?

RagingOcelot · 2017-04-15T20:47:55Z

src/client/app/actions/exportData.js

+	const encodedUri = encodeURI(csvContent);
+	const link = document.createElement('a');
+	let fileName = 'exportedDataOED';
+	fileName += '.csv';


Can do as one line?

…nnecessary conditional.

RagingOcelot

Looks good to me, thanks!

johndiiorio · 2017-04-16T00:49:47Z

src/client/app/services/exportData.js

+ */
+
+function convertToCSV(items) {
+	let csvOutput = 'id:,readings:,timestamp:\n';


You should change timestamp to start_timestamp and end_timestamp. You should also remove the colons in the labels.

johndiiorio · 2017-04-16T00:51:24Z

src/client/app/services/exportData.js

+		data.forEach(reading => {
+			const info = reading.y;
+			const timeStamp = moment(reading.x).format('dddd MMM DD YYYY hh:mm a');
+			csvOutput += `${id},${info} kwh, ${timeStamp} \n`;


In addition to the meter ID, I would add the name of the meter. As meter IDs are internal to our system, someone would have no idea what the meter ID represented.

I would even go so far as to think about not including the meterID at all. meterID is a surrogate key in our database and therefore doesn't mean anything outside the context of our system. There is no way it could help people consuming exported data. If it did it would be indicative of a bigger problem somewhere else in the export workflow.

johndiiorio · 2017-04-16T00:52:45Z

src/client/app/services/exportData.js

+	const fileName = 'exportedDataOED.csv';
+	link.setAttribute('href', encodedUri);
+	link.setAttribute('download', fileName);
+	document.body.appendChild(link);


I'm not an expert in this field, but is this the React way to do things? Also, after clicking the link, it might be prudent to remove this link.

This is definitely not the react way to do things. This throws an <a> element onto the page outside of the react root div, then clicks it. This is definitely not the right way to do things.
I could see maybe making an <a> element, clicking it, then removing it, but I'm sure there's a better way.

johndiiorio · 2017-04-16T00:54:11Z

src/client/app/services/exportData.js

+	const csvContent = `data:text/csv;charset=utf-8,${inputCSV}`;
+	const encodedUri = encodeURI(csvContent);
+	const link = document.createElement('a');
+	const fileName = 'exportedDataOED.csv';


Is there a more descriptive and "dynamic" name that we can give the file name? Perhaps include the start and end timestamps in the filename?

johndiiorio · 2017-04-16T00:57:04Z

src/client/app/containers/UIOptionsContainer.js

+	const timeInterval = state.graph.timeInterval;
+	const data = { datasets: [] };
+	for (const meterID of state.graph.selectedMeters) {
+		const readingsData = state.readings.byMeterID[meterID][timeInterval];


As of right now, this is completely correct. However, when the bar chart is added, readings will be retrieved from state.[line OR bar].readings...
Will the intention be to only grab line chart readings, bar chart readings, or both? If both, some additional logic will eventually be required here.

simonbtomlinson

Your overall logic looks good and you're interacting correctly with redux. My main concern is definitely UIOptionsComponent being a god object, and also the way that you make and click a link to make the user download a file. Good work overall. Just a few changes and this will be ready for master.

simonbtomlinson · 2017-04-16T01:50:47Z

src/client/app/components/UIOptionsComponent.jsx

+	 * Called when Export button is clicked.
+	 * Passes an object containing the selected meter data to a function for export.
+	 */
+	exportReading() {


I'm really not a fan of putting this logic in UIOptionsComponent. It's a discrete piece of functionality that only depends on redux state, so I think you should extract it to its own component and include that in UIOptionsComponent. UIOptionsComponent is really getting to be a god object that violates single responsibility principle and we should try to fight that hard. I think the entire thing should be refactored, but that's a matter for a different pull request.

simonbtomlinson · 2017-04-16T01:52:04Z

src/client/app/containers/UIOptionsContainer.js

@@ -14,9 +14,24 @@ import { fetchMetersDataIfNeeded } from '../actions/meters';
 */
 function mapStateToProps(state) {
 	const sortedMeters = _.sortBy(_.values(state.meters.byMeterID).map(meter => ({ id: meter.id, name: meter.name.trim() })), 'name');
+
+	const timeInterval = state.graph.timeInterval;
+	const data = { datasets: [] };


I'm not a fan of the ambiguous naming here. It's hard to figure out what part of UIOptionComponent's functionality this pertains to. Refactoring this into its own container/component should mostly fix this issue, but some more detailed naming (and a comment or two) might still be a good idea.

data seems to just be a wrapper around datasets. Why does it exist at all? Why not just use the datasets list as your variable?

simonbtomlinson · 2017-04-16T01:55:38Z

src/client/app/services/exportData.js

+		data.forEach(reading => {
+			const info = reading.y;
+			const timeStamp = moment(reading.x).format('dddd MMM DD YYYY hh:mm a');
+			csvOutput += `${id},${info} kwh, ${timeStamp} \n`;


I would even go so far as to think about not including the meterID at all. meterID is a surrogate key in our database and therefore doesn't mean anything outside the context of our system. There is no way it could help people consuming exported data. If it did it would be indicative of a bigger problem somewhere else in the export workflow.

simonbtomlinson · 2017-04-16T02:02:40Z

src/client/app/services/exportData.js

+	const fileName = 'exportedDataOED.csv';
+	link.setAttribute('href', encodedUri);
+	link.setAttribute('download', fileName);
+	document.body.appendChild(link);


This is definitely not the react way to do things. This throws an <a> element onto the page outside of the react root div, then clicks it. This is definitely not the right way to do things.
I could see maybe making an <a> element, clicking it, then removing it, but I'm sure there's a better way.

simonbtomlinson · 2017-04-16T02:04:40Z

src/server/services/readMetasysData.js

@@ -10,23 +10,23 @@ const Meter = require('./../models/Meter');
 async function readMetasysData(filePath) {


Thanks for cleaning this up a bit while you were doing other things. It slipped through linting somehow. I see there are a few other linting errors in here, but they aren't your code so it's no big deal.

simonbtomlinson · 2017-04-16T02:09:06Z

src/client/app/services/exportData.js

+
+function convertToCSV(items) {
+	let csvOutput = 'id:,readings:,timestamp:\n';
+	items.forEach(set => {


You use a for ... of loop in UIOptionsContainer.js, but .forEach here. Why the inconsistency?

# Conflicts: # src/client/app/actions/readings.js # src/client/app/components/UIOptionsComponent.jsx # src/client/app/containers/UIOptionsContainer.js # src/server/services/readMetasysData.js

…rently only export data from linecharts (its the same data as shown on the bar chart however it may include extra data points that were not included on the bar charts.

…a set export from linechart) timestamps for bar chart exports don't currently display correctly

…mestamp of the first exported reading.

kooolbreez1 · 2017-04-28T00:51:21Z

The export component now works with bar charts and the file name now changes based on the timestamp of the first reading. It still only exports each readings start time but as far as I can tell the charts only ever receive the start time of each reading. Maybe I am looking in the wrong place? I have also updated the actual save function to be a bit better but it essentially does the same thing as before.

simonbtomlinson · 2017-04-28T01:41:11Z

@kooolbreez1 You can get the current bar duration from state.graph.barDuration in redux. Then just add it to start points to get end points.

kooolbreez1 · 2017-04-28T01:43:07Z

that would work for the bar chart readings but not the line chart readings

simonbtomlinson · 2017-04-28T01:48:07Z

@kooolbreez1 Oh! Yeah, currently the only way to figure that out is to subtract the start of the second reading from the first one. They'll all have the same duration. We should probably start sending end timestamps to the client to do this more nicely.

johndiiorio

Almost good, just a couple of minor things.

johndiiorio · 2017-04-29T04:16:38Z

src/client/app/components/ExportComponent.jsx

+	}
+
+	render() {
+		return (<button onClick={this.exportReading}>Export!</button>);


Could use change this to a Bootstrap button? Preferably btn-default? Ideally, add the React-Bootstrap module and use it through that.

johndiiorio · 2017-04-29T04:19:00Z

src/client/app/containers/ExportContainer.js

+	const barDuration = state.graph.barDuration;
+
+	for (const meterID of state.graph.selectedMeters) {
+		if (chart === 'line') {


In reducers/graph.js, there is chartTypes. Even though it doesn't really do anything, you should use them instead of the string 'line' and 'bar' so that we don't get confused when we start adding more types of charts.

johndiiorio · 2017-04-29T04:19:10Z

src/client/app/containers/ExportContainer.js

+				timestamp: state.readings.line.byMeterID[meterID][timeInterval].start_timestamp,
+				exportVals: state.readings.line.byMeterID[meterID][timeInterval].readings.map(arr => ({ x: arr[0], y: arr[1] }))
+			});
+		} else if (readingsData !== undefined && !readingsData.isFetching && chart === 'bar') {


See above comment.

johndiiorio · 2017-04-29T04:21:32Z

src/client/app/components/ExportComponent.jsx

+		const compressedData = this.props.exportVals.datasets;
+		let time = compressedData[0].exportVals[0].x;
+		time = moment(time).format('ddddMMMDDYYYY');
+		const name = `oedExport${time}.csv`;


Could you add in whether it is bar or line data in the title? Also, if possibly, could you add the end time of the interval. So the whole thing could look like: oedExportLineFeb272017-March032017.csv

…rking with react-bootstrap however it is not formatted nicely compared to UIOptions. changed "line" and "bar" to chartType object.

kooolbreez1 · 2017-05-03T00:45:57Z

I believe I have made the changes that you requested @johndiiorio the only thing is that the export button doesn't display in line with the UIoptions panel, rather it appears in the bottom left corner of the screen

…more chart types were added

johndiiorio

Just one minor change!! Almost done.

johndiiorio · 2017-05-03T15:33:28Z

src/client/app/components/HomeComponent.jsx

@@ -15,6 +17,7 @@ export default function HomeComponent() {
 		<div>
 			<HeaderComponent renderLoginButton />
 			<DashboardContainer />
+			<ExportContainer />


Could you move this to be in UIOptionsComponent (at the bottom) instead of here?

…ixing the formatting errors.

kooolbreez1 · 2017-05-03T15:49:18Z

@johndiiorio All fixed, it is now formatted nicely.

simonbtomlinson

This looks almost perfect, and really my only concern is the functional component thing.

simonbtomlinson · 2017-05-03T18:01:02Z

src/client/app/components/ExportComponent.jsx

+import moment from 'moment';
+import graphExport from '../services/exportData';
+
+export default class ExportComponent extends React.Component {


This could easily be a functional component. No need for a class.

kooolbreez1 · 2017-05-04T00:52:14Z

I believe I have made the changes that @simonbtomlinson wanted.

simonbtomlinson · 2017-05-04T01:02:04Z

@kooolbreez1 I think so too. I'll take a look when I get a chance, tomorrow at the latest.

Thanks!

kooolbreez1 and others added 16 commits March 28, 2017 16:34

Changes to linechart component to enable export of data. currently ab…

aef9792

…le to return the name of the selection (as seen in the select meters window) and what i believe to be meter data of some kind (still need to confirm that this is meter data and not some abstraction)

Changes to linechart component to enable export of data. currently ab…

d3bb015

…le to return the name of the selection (as seen in the select meters window) and what i believe to be meter data of some kind (still need to confirm that this is meter data and not some abstraction)

Changes to LineChartComponent, Readings.js in actions and LineChartCo…

e381bd2

…ntainer to enable the export of the state object

did stuff

5f9996a

Merge branch 'master' into exportGroup

5e811fa

# Conflicts: # src/client/app/components/LineChartComponent.jsx # src/client/app/containers/LineChartContainer.js

Merge branch 'master' into exportGroup

c3f6752

# Conflicts: # src/client/app/actions/readings.js # src/client/app/components/LineChartComponent.jsx # src/client/app/containers/LineChartContainer.js

Merge remote-tracking branch 'origin/exportGroup' into exportGroup

7783382

# Conflicts: # src/client/app/actions/readings.js

Able to access (compressed?) readings via the UIOptionsComponent.

950b045

Able to format and download compressed meter data as a CSV file.

1d56556

Merge branch 'master' into exportGroup

539401e

Merge branch 'master' into exportGroup

d610906

Merge remote-tracking branch 'origin/exportGroup' into exportGroup

eda9804

added timestamp to exported Data

ff4f005

Merge branch 'master' into exportGroup

199d31d

Merge branch 'master' into exportGroup

5082823

changed name of the exported file to "exportedDataOED" instead of pro…

b659757

…mpting the user for a name. linted

sandeepacharya464 reviewed Apr 13, 2017

View reviewed changes

fixed a comment I accidentally changed in dashboardContainer

c3fc89f

kooolbreez1 assigned simonbtomlinson, jameeters, johndiiorio and RagingOcelot Apr 14, 2017

RagingOcelot suggested changes Apr 15, 2017

View reviewed changes

compressed filename variable in exportData.js to 1 line and removed u…

3ae6e54

…nnecessary conditional.

RagingOcelot approved these changes Apr 15, 2017

View reviewed changes

Moved exportData.js to new services directory.

40dea5e

johndiiorio suggested changes Apr 16, 2017

View reviewed changes

simonbtomlinson suggested changes Apr 16, 2017

View reviewed changes

preparing to merge with master

9db2746

kooolbreez1 added 5 commits April 23, 2017 16:24

Merging branch 'master' into exportGroup

3c22a47

# Conflicts: # src/client/app/actions/readings.js # src/client/app/components/UIOptionsComponent.jsx # src/client/app/containers/UIOptionsContainer.js # src/server/services/readMetasysData.js

Now works with the updated UIOptions component and barcharts. can cur…

6c0937f

…rently only export data from linecharts (its the same data as shown on the bar chart however it may include extra data points that were not included on the bar charts.

Successfully exports the displayed data from bar charts (for full dat…

dc03958

…a set export from linechart) timestamps for bar chart exports don't currently display correctly

Successfully exports the displayed data from bar charts (for full dat…

538b85c

…a set export from linechart) timestamps for bar chart exports don't currently display correctly

the name of the exported file now dynamically changes based on the ti…

c91b9a6

…mestamp of the first exported reading.

johndiiorio suggested changes Apr 29, 2017

View reviewed changes

Name of exported file includes its chartType, the export button is wo…

16f4e3a

…rking with react-bootstrap however it is not formatted nicely compared to UIOptions. changed "line" and "bar" to chartType object.

updated a line of code to mitigate a likely bug that would appear if …

4c6b4ec

…more chart types were added

johndiiorio suggested changes May 3, 2017

View reviewed changes

Export component is now rendered as art of the UI options component f…

a8ca745

…ixing the formatting errors.

johndiiorio approved these changes May 3, 2017

View reviewed changes

simonbtomlinson suggested changes May 3, 2017

View reviewed changes

ExportComponent is now a functional component instead of a class

394c155

simonbtomlinson approved these changes Sep 13, 2017

View reviewed changes

kooolbreez1 merged commit a3033a0 into master Sep 13, 2017

kooolbreez1 deleted the exportGroup branch September 13, 2017 15:43

NoraCodes mentioned this pull request Sep 13, 2017

Data Dump Export #101

Closed

huss unassigned simonbtomlinson, jameeters, RagingOcelot and johndiiorio Sep 27, 2017

		@@ -10,23 +10,23 @@ const Meter = require('./../models/Meter');
		async function readMetasysData(filePath) {

Export group #83

Export group #83

Conversation

kooolbreez1 commented Apr 13, 2017 • edited

sandeepacharya464 left a comment

Choose a reason for hiding this comment

kooolbreez1 commented Apr 13, 2017

RagingOcelot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RagingOcelot 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

simonbtomlinson 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

kooolbreez1 commented Apr 28, 2017

simonbtomlinson commented Apr 28, 2017 • edited

kooolbreez1 commented Apr 28, 2017

simonbtomlinson commented Apr 28, 2017

johndiiorio 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

johndiiorio Apr 29, 2017 • edited

Choose a reason for hiding this comment

kooolbreez1 commented May 3, 2017

johndiiorio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kooolbreez1 commented May 3, 2017

simonbtomlinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kooolbreez1 commented May 4, 2017

simonbtomlinson commented May 4, 2017

kooolbreez1 commented Apr 13, 2017 •

edited

simonbtomlinson commented Apr 28, 2017 •

edited

johndiiorio Apr 29, 2017 •

edited