From 6786260f04e7abd6c3d92718eaadac734f3e5ec4 Mon Sep 17 00:00:00 2001 From: Justin Walgran Date: Wed, 25 Mar 2020 08:00:01 -0700 Subject: [PATCH 1/4] Return contributor details from list API and include in CSV download The complex permission and visibility checks required make fetching contributor details for each facility slow, so we are cutting the maximum results per page from 500 to 50. Contributor names and list names can have commas, so we join them with a vertical bar in the CSV to make machine parsing of multiple contributors possible. --- .../__tests__/utils.facilitiesCSV.tests.js | 21 +++++++ .../src/components/FilterSidebarSearchTab.jsx | 2 +- src/app/src/util/constants.js | 2 +- src/app/src/util/util.facilitiesCSV.js | 3 + src/django/api/pagination.py | 4 +- src/django/api/serializers.py | 55 +++++++++---------- src/django/api/views.py | 21 ++++++- 7 files changed, 73 insertions(+), 35 deletions(-) diff --git a/src/app/src/__tests__/utils.facilitiesCSV.tests.js b/src/app/src/__tests__/utils.facilitiesCSV.tests.js index 1603443a4..0d097b570 100644 --- a/src/app/src/__tests__/utils.facilitiesCSV.tests.js +++ b/src/app/src/__tests__/utils.facilitiesCSV.tests.js @@ -15,6 +15,18 @@ it('creates a new facility row array from a feature', () => { country_code: 'country_code', country_name: 'country_name', oar_id: 'oar_id', + contributors: [ + { + id: 1, + name: 'contributor_name (list_name)', + verified: false, + }, + { + id: 2, + name: 'contributor_2_name (list_2_name)', + verified: true, + }, + ], }, geometry: { coordinates: [ @@ -32,6 +44,7 @@ it('creates a new facility row array from a feature', () => { 'country_name', 'lat', 'lng', + 'contributor_name (list_name)|contributor_2_name (list_2_name)', ]; expect(isEqual( @@ -49,6 +62,13 @@ it('creates a 2-d array including headers for exporting as a CSV', () => { country_code: 'country_code', country_name: 'country_name', oar_id: 'oar_id', + contributors: [ + { + id: 1, + name: 'contributor_name', + verified: false, + }, + ], }, geometry: { coordinates: [ @@ -69,6 +89,7 @@ it('creates a 2-d array including headers for exporting as a CSV', () => { 'country_name', 'lat', 'lng', + 'contributor_name', ], ]; diff --git a/src/app/src/components/FilterSidebarSearchTab.jsx b/src/app/src/components/FilterSidebarSearchTab.jsx index 68c5b0679..f4ff17d01 100644 --- a/src/app/src/components/FilterSidebarSearchTab.jsx +++ b/src/app/src/components/FilterSidebarSearchTab.jsx @@ -446,7 +446,7 @@ function mapDispatchToProps(dispatch, { return dispatch(resetAllFilters()); }, searchForFacilities: vectorTilesAreActive => dispatch(fetchFacilities({ - pageSize: vectorTilesAreActive ? FACILITIES_REQUEST_PAGE_SIZE : 500, + pageSize: vectorTilesAreActive ? FACILITIES_REQUEST_PAGE_SIZE : 50, pushNewRoute: push, })), submitFormOnEnterKeyPress: makeSubmitFormOnEnterKeyPressFunction( diff --git a/src/app/src/util/constants.js b/src/app/src/util/constants.js index cdc276489..9c2dadade 100644 --- a/src/app/src/util/constants.js +++ b/src/app/src/util/constants.js @@ -1,6 +1,6 @@ export const OTHER = 'Other'; -export const FACILITIES_REQUEST_PAGE_SIZE = 100; +export const FACILITIES_REQUEST_PAGE_SIZE = 50; // This choices must be kept in sync with the identical list // kept in the Django API's models.py file diff --git a/src/app/src/util/util.facilitiesCSV.js b/src/app/src/util/util.facilitiesCSV.js index 334380436..d95504cfc 100644 --- a/src/app/src/util/util.facilitiesCSV.js +++ b/src/app/src/util/util.facilitiesCSV.js @@ -11,6 +11,7 @@ export const csvHeaders = Object.freeze([ 'country_name', 'lat', 'lng', + 'contributors', ]); export const createFacilityRowFromFeature = ({ @@ -20,6 +21,7 @@ export const createFacilityRowFromFeature = ({ country_code, country_name, oar_id, + contributors, }, geometry: { coordinates: [ @@ -35,6 +37,7 @@ export const createFacilityRowFromFeature = ({ country_name, lat, lng, + contributors ? contributors.map(c => c.name).join('|') : '', ]); export const facilityReducer = (acc, next) => diff --git a/src/django/api/pagination.py b/src/django/api/pagination.py index 9c89efdce..7e6f872f2 100644 --- a/src/django/api/pagination.py +++ b/src/django/api/pagination.py @@ -11,5 +11,5 @@ class PageAndSizePagination(PageNumberPagination): class FacilitiesGeoJSONPagination(GeoJsonPagination): page_query_param = 'page' page_size_query_param = 'pageSize' - page_size = 500 - max_page_size = 500 + page_size = 50 + max_page_size = 50 diff --git a/src/django/api/serializers.py b/src/django/api/serializers.py index 448371cdc..e762fd8a1 100644 --- a/src/django/api/serializers.py +++ b/src/django/api/serializers.py @@ -416,11 +416,12 @@ def validate_status(self, value): class FacilitySerializer(GeoFeatureModelSerializer): oar_id = SerializerMethodField() country_name = SerializerMethodField() + contributors = SerializerMethodField() class Meta: model = Facility fields = ('id', 'name', 'address', 'country_code', 'location', - 'oar_id', 'country_name') + 'oar_id', 'country_name', 'contributors') geo_field = 'location' # Added to ensure including the OAR ID in the geojson properties map @@ -430,13 +431,33 @@ def get_oar_id(self, facility): def get_country_name(self, facility): return COUNTRY_NAMES.get(facility.country_code, '') + def get_contributors(self, facility): + def format_source(source): + if type(source) is Source: + return { + 'id': source.contributor.admin.id + if source.contributor else None, + 'name': source.display_name, + 'is_verified': source.contributor.is_verified + if source.contributor else False, + } + return { + 'name': source, + } + request = self.context.get('request') \ + if self.context is not None else None + user = request.user if request is not None else None + return [ + format_source(source) + for source + in facility.sources(user=user) + ] + -class FacilityDetailsSerializer(GeoFeatureModelSerializer): - oar_id = SerializerMethodField() +class FacilityDetailsSerializer(FacilitySerializer): other_names = SerializerMethodField() other_addresses = SerializerMethodField() other_locations = SerializerMethodField() - contributors = SerializerMethodField() country_name = SerializerMethodField() claim_info = SerializerMethodField() @@ -447,10 +468,6 @@ class Meta: 'country_name', 'claim_info', 'other_locations') geo_field = 'location' - # Added to ensure including the OAR ID in the geojson properties map - def get_oar_id(self, facility): - return facility.id - def get_other_names(self, facility): return facility.other_names() @@ -499,28 +516,6 @@ def get_other_locations(self, facility): return facility_locations + facility_matches - def get_contributors(self, facility): - def format_source(source): - if type(source) is Source: - return { - 'id': source.contributor.admin.id - if source.contributor else None, - 'name': source.display_name, - 'is_verified': source.contributor.is_verified - if source.contributor else False, - } - return { - 'name': source, - } - request = self.context.get('request') \ - if self.context is not None else None - user = request.user if request is not None else None - return [ - format_source(source) - for source - in facility.sources(user=user) - ] - def get_country_name(self, facility): return COUNTRY_NAMES.get(facility.country_code, '') diff --git a/src/django/api/views.py b/src/django/api/views.py index c030440d0..f8f865d48 100644 --- a/src/django/api/views.py +++ b/src/django/api/views.py @@ -646,7 +646,14 @@ def list(self, request): "address" "facility address_1", "country_code": "US", "country_name": "United States", - "oar_id": "OAR_ID_1" + "oar_id": "OAR_ID_1", + "contributors": [ + { + "id": 1, + "name": "Brand A (2019 Q1 List)", + "is_verified": false + } + ] } }, { @@ -662,6 +669,18 @@ def list(self, request): "country_code": "US", "country_name": "United States", "oar_id": "OAR_ID_2" + "contributors": [ + { + "id": 1, + "name": "Brand A (2019 Q1 List)", + "is_verified": false + }, + { + "id": 2, + "name": "MSI B (2020 List)", + "is_verified": true + } + ] } } ] From 32eae9f31b2de9af1ae4467026af006592932f06 Mon Sep 17 00:00:00 2001 From: Justin Walgran Date: Fri, 27 Mar 2020 11:52:39 -0700 Subject: [PATCH 2/4] Prevent contributor names and list names from containing | We are using the | character to separate contributors in CSV downloads so we want to prevent using that character in names of contributors or lists. --- src/django/api/views.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/django/api/views.py b/src/django/api/views.py index f8f865d48..b51cd9af0 100644 --- a/src/django/api/views.py +++ b/src/django/api/views.py @@ -161,6 +161,9 @@ def create(self, request, *args, **kwargs): if name is None: raise ValidationError('name cannot be blank') + if '|' in name: + raise ValidationError('name cannot contain the "|" character') + if description is None: raise ValidationError('description cannot be blank') @@ -293,6 +296,9 @@ def put(self, request, pk, *args, **kwargs): if name is None: raise ValidationError('name cannot be blank') + if '|' in name: + raise ValidationError('Name cannot contain the "|" character.') + if description is None: raise ValidationError('description cannot be blank') @@ -1864,6 +1870,9 @@ def create(self, request): else: name = os.path.splitext(csv_file.name)[0] + if '|' in name: + raise ValidationError('Name cannot contain the "|" character.') + if 'description' in request.data: description = request.data['description'] else: From 77a4ea1510131c304077059f388804cb80701268 Mon Sep 17 00:00:00 2001 From: Justin Walgran Date: Fri, 27 Mar 2020 13:08:36 -0700 Subject: [PATCH 3/4] Show a determinate progress bar when download facility CSV We know the total number of facilities so it is possible to show percent progress. We have opted for a linear progress bar rather than a circular one because it looks better in this situation where our progress jumps rather than smoothly increases. --- .../src/components/FilterSidebarFacilitiesTab.jsx | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/app/src/components/FilterSidebarFacilitiesTab.jsx b/src/app/src/components/FilterSidebarFacilitiesTab.jsx index f78e26159..938b837c2 100644 --- a/src/app/src/components/FilterSidebarFacilitiesTab.jsx +++ b/src/app/src/components/FilterSidebarFacilitiesTab.jsx @@ -3,6 +3,7 @@ import { arrayOf, bool, func, number, string } from 'prop-types'; import { connect } from 'react-redux'; import { Link } from 'react-router-dom'; import CircularProgress from '@material-ui/core/CircularProgress'; +import LinearProgress from '@material-ui/core/LinearProgress'; import Dialog from '@material-ui/core/Dialog'; import DialogTitle from '@material-ui/core/DialogTitle'; import DialogContent from '@material-ui/core/DialogContent'; @@ -82,6 +83,9 @@ const facilitiesTabStyles = Object.freeze({ height: '45px', margin: '5px 0', }), + downloadLabelStyles: Object.freeze({ + margin: '5px 0', + }), }); function FilterSidebarFacilitiesTab({ @@ -191,6 +195,10 @@ function FilterSidebarFacilitiesTab({ const LoginLink = props => ; const RegisterLink = props => ; + const progress = facilitiesCount + ? get(data, 'features', []).length * 100 / facilitiesCount + : 0; + const listHeaderInsetComponent = (
- +
+ Downloading... +
+
) : (