958 export csv #2850

fnocetti · 2020-04-02T13:29:15Z

fixes #958

PR checklist:

Update READ.me ?
Update API documentation ?

QA checklist:

RafaPolit · 2020-04-02T19:14:10Z

Still very much ongoing review, but I am seeing a lot of :any types on the .ts files. I understand it's the quickest way, but not sure if we should make an extra attempt to not do that unless there is no other way around? It kind of defeats the purpose of the typing.

On tests I think we can be much more lenient, but on actual code, I think it's important to try to capture the actual types of variables? What do you think @daneryl @konzz?

fnocetti · 2020-04-02T19:33:32Z

@RafaPolit I actually agree with you! Most of the any-typed variables refer to arbitrary objects for which I will need to define types/interfaces (because, right, it was quicker to implement). But now since the code has its iterated design I should be able to make strong typing my next priority after unit-testing.

As for the SearchResults type: I couldn't find a type for that in the application. Do you know if we have a type definition? If not I will continue to expand the one I'm using. It won't probably be complete enough to put it under the shared/types (or similar) directory, IMHO. Do you thing it's worth to define it as an application-wide type?

RafaPolit · 2020-04-02T19:27:49Z

app/api/csv/typeFormatters.ts

+  [key: string]: (field: any, options?: any) => string;
+} = {
+  select: (field: any) => (field && field[0] ? field[0].label : ''),
+  multiselect: (field: any) => field?.map((item: any) => formatters.select([item])).join('|') || '',


I am confused. How can .join('|') be undefined or null, so that this will require an OR here? Perhaps I'm misreading this? Still, that is one the issues of not doing the tests first, we tend to code for non-existing scenarios.

Also, apparently every other scenario assumes field can be undefined or null (is that even a correct assumption?), yet this one just 'discards' that option with the ? in field. So, which is correct? It can be undefined and this will fail, or all others have a redundant check that is not needed? Please comment. Thanks.

I was not doing the checks at first, but I got errors because I got rows in the results for which the template indicated there should be a field, while it was actually not set in the row. The OR operand is related to the optional chaining of map. It will make the whole instruction return an undefined value and the OR defaults it to the empty string.

Anyways, this is clearly not the place to do those checks as the formatters should only transform an existing field. I'll remove them from here and try to reproduce the failing scenarios.

RafaPolit · 2020-04-02T19:31:03Z

app/api/csv/typeFormatters.ts

+  link: (field: any) => (field && field[0] ? `${field[0].value.label}|${field[0].value.url}` : ''),
+  media: (field: any) => (field && field[0] ? field[0].value : ''),
+  multidate: (field: any, { dateFormat }) =>
+    field?.map((item: any) => formatters.date([item], dateFormat)).join('|') || '',


Same comment as above, not sure how a join can return undefined. It can return an empty string, which would trigger the conditional to return an empty string? This is happening in some other multi-values below.

RafaPolit · 2020-04-02T20:27:28Z

app/api/files/jsRoutes.js

+    '/api/export',
+
+    validation.validateRequest(
+      Joi.object().keys({


@daneryl should we start using the new JSON schema here or keep JOI for old route files (even if the endpoint is new)?

@RafaPolit @fnocetti i think we should remove gradually the joi validations, for sure for new routes use the new one, example here:

uwazi/app/api/files/routes.ts

Lines 98 to 106 in 988b5dd

validation.validateRequest({

properties: {

query: {

properties: {

_id: { type: 'string' },

},

},

},

}),

RafaPolit · 2020-04-02T20:30:58Z

app/api/files/jsRoutes.js

+                    return handleError(err);
+                  }
+
+                  return fs.unlinkSync(temporalFilePath);


Even if this could be fast, it is still a blocking procedure. While using this in tests may work, please us non-sync unlink here. Also, fs.unlink returns undefined, so be sure this is the expected behavior for success download.

RafaPolit · 2020-04-02T20:35:58Z

app/react/Library/actions/exportActions.js

@@ -0,0 +1,76 @@
+import superagent from 'superagent';


If this is a completely new file, should this be a TS file?

RafaPolit · 2020-04-02T20:37:56Z

app/react/Library/actions/exportActions.js

+}
+
+export function exportDocuments(storeKey) {
+  // eslint-disable-next-line max-statements


Please refactor this to contain less statements instead of disabling the lint rule.

RafaPolit · 2020-04-02T20:38:06Z

app/api/files/jsRoutes.js

+              });
+            })
+            .catch(e => {
+              fs.unlinkSync(temporalFilePath);


Again, use regular unlink.

RafaPolit · 2020-04-02T20:39:39Z

app/react/Library/components/ExportButton.js

@@ -0,0 +1,52 @@
+import PropTypes from 'prop-types';


A new component should be a TS file.

RafaPolit · 2020-04-02T20:43:58Z

app/react/Library/reducers/exportReducer.js

@@ -0,0 +1,9 @@
+import { combineReducers } from 'redux';


Make this file TS.

RafaPolit · 2020-04-02T20:44:06Z

app/react/Library/reducers/exportReducer.js

+import createReducer from 'app/BasicReducer';
+
+export default combineReducers({
+  exportProcessing: createReducer('exportProcessing', false),


I'm wondering if this should be better namespaced? Perhaps exportCSVProcessing? or exportLibraryProcessing? Also, there should be consistent naming across the store, because the exportEntity store entry point is missleading. This is exportSearchResults, or exportLibrary. ExportEntity should be a 'single' entity export and reserved for a different flow.

Yes, makes sense. I'll take care of it.

RafaPolit · 2020-04-02T20:48:36Z

As for the SearchResults type: I couldn't find a type for that in the application. Do you know if we have a type definition? If not I will continue to expand the one I'm using. It won't probably be complete enough to put it under the shared/types (or similar) directory, IMHO. Do you thing it's worth to define it as an application-wide type?

It probably is, not sure if this should be done at this point. For sure, we want typing of our search results.

RafaPolit · 2020-04-20T17:16:14Z

app/api/csv/csvExporter.ts

+import translations from 'api/i18n/translations';
+import formatters from './typeFormatters';
+
+export type SearchResults = {


Everything is exported: all functions, all types. Is this really necessary? The specs only import about 12 of the 16 exports here, and external modules should probably only import the default.

You're right. It also suggests that the testing strategy might be wrong. I propose you not exporting the elements that are not explicitly tested right away, and assuming the tech debt of refactoring this class.

RafaPolit · 2020-04-20T17:18:15Z

app/api/csv/csvExporter.ts

+};
+
+export const getTypes = (searchResults: SearchResults, typesWhitelist: string[] = []) =>
+  typesWhitelist.length > 0


Unnecessary compare operator here: typesWhitelist.length is enough, as it will be true if > 0 and false if not.

RafaPolit · 2020-04-20T17:25:25Z

app/api/csv/csvExporter.ts

+  collection.findIndex((i: any) => Object.keys(i).every(key => i[key] === item[key])) < 0;
+
+export const excludedProperties = (property: PropertySchema) =>
+  !['geolocation', 'preview', 'markdown', 'nested'].includes(property.type);


Does this mean that geolocation properties are also not included? Or are they processed elsewhere?

Answer: #2850 (comment)

RafaPolit · 2020-04-20T17:27:23Z

app/api/csv/csvExporter.ts

+
+export const concatCommonHeaders = (headers: ExportHeader[]) =>
+  headers.concat([
+    { label: 'Geolocation', name: 'geolocation', common: true },


While Documents, Attachments and Published are 'higher order' properties, the _geolocation properties reside within the metadata, they are not common to all templates, and there can be several with different labels for a single template. I'm having a hard time following the process of geolocation. I think there is something wrong in the concept here.

The geolocation property is treated as a special case because I found there is some different logic going on in its displaying across the app. I may be totally wrong and had misunderstood the concept. Let's have a chat asap.

RafaPolit · 2020-04-20T17:33:17Z

app/api/csv/specs/csvExporter.spec.ts

+      {
+        _id: '58ad7d240d44252fee4e6200',
+        id: 'f6c30c8a-3013-471f-be8c-e639f6aeb031',
+        name: 'company',
+        type: 'text',
+        label: 'company',
+      },
+      {
+        _id: '58ad7d240d44252fee4e61ff',
+        relationType: '5aae90e0bfbf88e5ae28b19e',
+        id: '3780239a-f858-4df5-9dea-8ea4a59cfc9e',
+        name: 'nemesis',
+        content: '58ad7d240d44252fee4e61fb',
+        type: 'relationship',
+        label: 'Nemesis',
+      },
+      {
+        _id: '5e3d19ccdeeb2652690a1258',
+        label: 'Costume',
+        type: 'select',
+        content: '5e3d1853deeb2652690a0c10',
+        showInCard: false,
+        filter: false,
+        name: 'costume',
+        id: 'a5407e10-5148-4241-ad81-0c8fa78f1c43',
+      },
+      {
+        _id: '58ad7d240d44252fee4e61fe',
+        filter: true,
+        id: '113d9a13-7fb9-447f-abf1-4075a9f8eb00',
+        name: 'super_powers',
+        content: '58ad7d240d44252fee4e6208',
+        type: 'multiselect',
+        label: 'Super powers',
+      },
+      {
+        _id: '59859ad8ddb12b0ce6664927',
+        relationType: '5a8480fac464318833d9b553',
+        label: 'Allies',
+        type: 'relationship',
+        content: '58ad7d240d44252fee4e61fb',
+        name: 'allies',
+        id: 'c22326ac-6723-42b5-bb3e-de0fdcc2e2dc',
+      },
+      {
+        nestedProperties: [],
+        _id: '5e8f9509f16db8b791fec574',
+        label: 'Geolocation',
+        type: 'geolocation',
+        name: 'geolocation_geolocation',
+        id: 'e9b810a9-8f25-442f-b521-616f3f3bbcdd',
+      },
+    ],
+  },
+  '58ad7d240d44252fee4e61fb': {
+    _id: '58ad7d240d44252fee4e61fb',
+    name: 'Super Villian',
+    properties: [
+      {
+        _id: '5e3d1880deeb2652690a1036',
+        label: 'Costume',
+        type: 'select',
+        content: '5e3d1853deeb2652690a0c10',
+        name: 'costume',
+        id: '53d6bb4a-2819-47b5-95a3-9261da5e8a69',
+      },
+      {
+        _id: '58ad7d240d44252fee4e61fc',
+        id: '58dd46d2-b52c-4e80-a859-6f4fadabe4c0',
+        name: 'super_powers',
+        content: '58ad7d240d44252fee4e6208',
+        type: 'multiselect',
+        label: 'Super powers',
+      },
+      {
+        _id: '594bc3b0bee8b3829aea937f',
+        relationType: '5aae90e0bfbf88e5ae28b1a3',
+        label: 'Sidekick',
+        type: 'relationship',
+        content: '58f0aed2e147e720856a0741',
+        name: 'sidekick',
+        id: '0c45e6dc-0081-463e-9300-c46d13b1dcd2',
+      },
+      {
+        _id: '594bc3b0bee8b3829aea937e',
+        label: 'Planets conquered',
+        type: 'numeric',
+        name: 'planets_conquered',
+        id: '6ec2c27a-ec30-4d0f-a7da-217a57e40ef2',
+      },
+      {
+        _id: '594bc3b0bee8b3829aea937d',
+        label: 'DOB',
+        type: 'date',
+        name: 'dob',
+        id: '87289f51-21fb-4ff6-bb8d-8b7d66d86526',
+      },
+    ],
+  },
+};


Perhaps this fixtures are long enough to merit a 'fixtrues' file like in many other api tests?

RafaPolit · 2020-04-20T17:43:17Z

app/react/Library/components/ExportButton.tsx

+  exportDocuments: (keyStore: string) => any;
+};
+
+export class ExportButton extends Component<ExportButtonProps, {}> {


Do not export the component if it is connected. You can import the connected component in the test and either use the .WrappedComponent property of the component, or you can use '.dive()' methods to inject the store into the component. That also allows you to test the mapStateToProps directly, which in this case are not tested at all.

You can check the dive method on /app/react/components/Elements/specs/FeatureToggle.spec.tsx

RafaPolit · 2020-04-20T17:45:40Z

app/react/UI/Icon/export-csv.js

+/**
+ *
+ *
+ * @format
+ */


This is no longer required, please remove.

RafaPolit · 2020-04-20T17:47:01Z

app/react/UI/Icon/library.js

@@ -88,6 +88,7 @@ import { faUsers } from '@fortawesome/free-solid-svg-icons/faUsers';
 import { faUserTimes } from '@fortawesome/free-solid-svg-icons/faUserTimes';
 import { faHandPaper } from '@fortawesome/free-solid-svg-icons/faHandPaper';
 import { saveAndNext } from './save-and-next';
+import { exportCsv } from './export-csv';


Was there really no icon in the entire font-awesome library that we had to create our own icon? Couldn't we have picked an icon from the library and added it here? Can you elaborate on how this happened? Thanks.

It was requested by @simonfossom from his work in the new look and feel. There is a download/export icon that could be useful but the icon he proposed is a merge between a file download and a grid that better represent a CSV export.
Maybe we can have a conversation about that if we prefer avoiding to create custom icons.

RafaPolit · 2020-04-20T17:50:48Z

package.json

+    "@types/fetch-mock": "^7.3.2",
+    "@types/hoist-non-react-statics": "^3.3.1",


If you require 3.3.1, please also update the hoist-non-react-statistics to the ^3.3.1 please, lets not have a discrepancy of types and installed version.

fnocetti · 2020-04-21T19:41:36Z

I found a bug that I need to take care of.

RafaPolit · 2020-05-04T18:40:39Z

This LGTM.

fnocetti force-pushed the 958-export-csv branch from 1f501d7 to ae54746 Compare April 2, 2020 17:12

RafaPolit requested changes Apr 2, 2020

View reviewed changes

fnocetti force-pushed the 958-export-csv branch from 17f666d to 0bd0f74 Compare April 15, 2020 17:24

fnocetti changed the title ~~958 export csv (WIP)~~ 958 export csv Apr 17, 2020

fnocetti added 22 commits April 17, 2020 11:39

Generate endpoint structure

b9a33d3

Export service exporting to stdout

feddaaf

Added download file support and bug fixing

a0498c3

fix bug on header computation

6198bc8

wip export csv ui

b6ebf50

Polish UI integration, add published field, bugfixes

1976658

Move status to store, remove support for ws. Bugfix.

0e1726c

Fix eslint problem and remove unused code

f5a6970

Unit tests and fixes for typeFormatters

7e16aaf

Requested changes and unit test refactor

971cc1e

Unit tests and minor changes

9095034

Add fetch-mock types to dev

b342292

Add unit test and bug fixes

11688e1

Fix unit tests

d1f6e83

Fix dates formatters

0841662

Fix dates

284e041

ExportButton tests

962a7c0

Minor fixes

99a7049

Bugfix and translation support

682ddfc

Route tests

6c58bad

Fix codeclimate

c055ce6

Fix broken test suite

7d95a1e

fnocetti force-pushed the 958-export-csv branch from 6b40f7c to 7d95a1e Compare April 17, 2020 15:06

fnocetti added the Ready for QA label Apr 20, 2020

RafaPolit reviewed Apr 20, 2020

View reviewed changes

konzz assigned RafaPolit Apr 21, 2020

PR discussed changes

66a0733

fnocetti removed the Ready for QA label Apr 21, 2020

fnocetti added 4 commits April 21, 2020 20:10

Markdown fields support

f634a75

Fix typing on test

3b0efdb

Bug fix: Writing incomplete file to disk

2f10299

Fix codeclimate

48e0381

fnocetti added the Ready for QA label Apr 22, 2020

Merge branch 'development' into 958-export-csv

493ee86

RafaPolit merged commit 4471950 into development May 4, 2020

RafaPolit deleted the 958-export-csv branch May 4, 2020 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

958 export csv #2850

958 export csv #2850

fnocetti commented Apr 2, 2020

RafaPolit commented Apr 2, 2020 •

edited

Loading

fnocetti commented Apr 2, 2020

RafaPolit Apr 2, 2020

fnocetti Apr 2, 2020

RafaPolit Apr 2, 2020

RafaPolit Apr 2, 2020 •

edited

Loading

daneryl Apr 3, 2020

RafaPolit Apr 2, 2020

RafaPolit Apr 2, 2020

RafaPolit Apr 2, 2020

RafaPolit Apr 2, 2020

RafaPolit Apr 2, 2020

RafaPolit Apr 2, 2020

RafaPolit Apr 2, 2020

fnocetti Apr 2, 2020

RafaPolit commented Apr 2, 2020

RafaPolit Apr 20, 2020

fnocetti Apr 21, 2020

RafaPolit Apr 20, 2020 •

edited

Loading

RafaPolit Apr 20, 2020

fnocetti Apr 21, 2020

RafaPolit Apr 20, 2020

fnocetti Apr 21, 2020

RafaPolit Apr 20, 2020

RafaPolit Apr 20, 2020

RafaPolit Apr 20, 2020

RafaPolit Apr 20, 2020

fnocetti Apr 20, 2020

RafaPolit Apr 20, 2020

fnocetti commented Apr 21, 2020

RafaPolit commented May 4, 2020

	validation.validateRequest({
	properties: {
	query: {
	properties: {
	_id: { type: 'string' },
	},
	},
	},
	}),

		"@types/fetch-mock": "^7.3.2",
		"@types/hoist-non-react-statics": "^3.3.1",

958 export csv #2850

958 export csv #2850

Conversation

fnocetti commented Apr 2, 2020

RafaPolit commented Apr 2, 2020 • edited Loading

fnocetti commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafaPolit Apr 2, 2020 • edited Loading

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

RafaPolit commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafaPolit Apr 20, 2020 • edited Loading

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

fnocetti commented Apr 21, 2020

RafaPolit commented May 4, 2020

RafaPolit commented Apr 2, 2020 •

edited

Loading

RafaPolit Apr 2, 2020 •

edited

Loading

RafaPolit Apr 20, 2020 •

edited

Loading