go-frontend icon indicating copy to clipboard operation
go-frontend copied to clipboard

Published 20 hours ago • verified publisher icon IFRCGo

Remove hard-coded values and database ids from frontend code

Open batpad opened this issue 5 years ago • 15 comments

There are various places on the frontend where values are hard-coded as JSON, for example: https://github.com/IFRCGo/go-frontend/blob/develop/app/assets/scripts/utils/field-report-constants.js#L33

This is extremely brittle, and makes it harder for us to make data updates on the backend, since that would break hard-coded values on the frontend.

There are a few places where this pattern exists. We need to replace all of them to fetch values via API calls to the database. Ideally, we would also maintain a cache of these commonly fetched values, like a list of countries, in localStorage or so.

cc @necoline @frozenhelium

batpad avatar Jan 24 '20 11:01 batpad

Similar to https://github.com/IFRCGo/go-api/issues/379 on back-end, would be fine to clean up.

szabozoltan69 avatar Jan 24 '20 12:01 szabozoltan69

@batpad confirms this is a top priority for refactor work in 4.2.

FYI #938 has links to a number of tickets around countries (although that ticket is much wider than just the front end/back end thing!)

nanometrenat avatar Feb 24 '20 12:02 nanometrenat

Here is a list of places where hard-coded values should be replaced with API calls: I would love to see if there are existing API calls that we can immediately put into place here or if we need to make a PR to create that

https://github.com/IFRCGo/go-frontend/blob/develop/app/assets/scripts/

country-codes:

per-components:

region-constants:

fdrs-codes:

whitelistedDomains:

country-iso:

country-bounding-boxes:

country-centroids:

district-centroids:

eru-types:

necoline avatar Feb 27 '20 15:02 necoline

Hi @necoline thanks for this.

Re some of your points: I think country-codes and country-iso are returned by /api/v2/country/ BUT we need to make sure we have the right filtering in place for each usage (as Regional Offices etc. are also stored in country table and returned via API.

Re ERU-types I think this can be ignored for now as will shortly be rejigging the ERU section.

nanometrenat avatar Feb 27 '20 17:02 nanometrenat

Got it. Thanks @nanometrenat! This is very helpful

necoline avatar Feb 27 '20 17:02 necoline

@necoline this is great!

For country-bounding-boxes, country-centroids and district-centroids, I am fairly certain we currently don't store this data on the backend. I know this is marked as low priority as I can see there is less chance of things breaking, but I would strongly advocate adding these to the backend.

@necoline @nanometrenat we can talk about this, but I think this will be an important step toward overall geospatial data handling by consolidating our current data on the backend and is very much worth doing. I will ticket out the steps required on the backend.

batpad avatar Feb 27 '20 17:02 batpad

Thanks team! @batpad agree we should absolutely consolidate rather than have things in two places - however if data is stored in only one place (front end) and is rarely changed then we need to consider site performance when deciding whether or not to store it in the back end instead. We wouldn't want to make page loads any slower if they don't need to be. Know you'll consider that (e.g. caching options) but thought it worth mentioning! Thanks heaps

nanometrenat avatar Feb 27 '20 18:02 nanometrenat

From my perspective, the current approach is not saving us on performance since it is not connected to any caching strategy that I am aware of. Putting this in the backed will allow us to pursue best practices for performant request strategies that can be broadly applied to other data points that are rarely updated.

necoline avatar Feb 28 '20 15:02 necoline

+1 - Alert types, Alert categories in app/assets/scripts/components/connected/alerts-table.js

GergiH avatar Mar 17 '20 15:03 GergiH

This one needs to done in order to complete the translation work. All the labels in the front-end needs to be in sync with back-end. We need to take a look into this and see how much of work is this one. I'll get back once I estimate on it.

CC: @thenav56 @batpad @nanometrenat

frozenhelium avatar Jul 08 '20 11:07 frozenhelium

@frozenhelium from the above list, we will be moving the geospatial related entities (countries, centroids, etc.) to the backend in the current phase of work. Let's discuss what you folks would need for translation and prepare accordingly, and also check what else would need to be moved that's important for translation. Thanks for flagging.

batpad avatar Jul 08 '20 12:07 batpad

Just a quick update on this ticket: Thanks to @frozenhelium, we have a good pattern now to fetch this data during initial app load, without adding too much complexity to each individual component that depends on these values: https://github.com/IFRCGo/go-frontend/pull/1404 .

Now it is a matter of ensuring all the data currently hard-coded is available via API calls, and managing setting and getting that data from the state rather than hard-coded JSON defined in the javascript. For countries / districts, etc. this means having the spatial data available in the backend, which we should have soon.

Hopefully, then switching the frontend over to using values from the API is fairly quick thanks to @frozenhelium's work in #1404 .

batpad avatar Jul 24 '20 06:07 batpad

Quick update -- all the geospatial related hardcoded ids and codes (except for fdrs) are now being deprecated via https://github.com/IFRCGo/go-frontend/pull/1514

The deprecated items are:

  • countryList from field-report-constants.js
  • countryIsoMapById from field-report-constants.js
  • countryIsoMapByName from field-report-constants.js
  • countryNameMapById from field-report-constants.js
  • countryNameMapByIso from field-report-constants.js
  • src/root/utils/region-bounding-box.js
  • src/root/utils/region-constants.js
  • src/root/utils/country-bounding-box.js
  • src/root/utils/country-centroids.js
  • src/root/utils/country-iso.js
  • src/root/utils/district-centroids.js

geohacker avatar Aug 18 '20 11:08 geohacker

Hey @batpad. Just doing some ticket gardening. With @geohacker comment above, I think all geo aspects of this are now sorted but there may be others that still require attention. I'm taking off my 'geo list' but won't close.

jhenshall avatar Oct 07 '20 09:10 jhenshall

@frozenhelium @batpad @tovari did this get addressed in the GO rewrite or is it still an open thing for build? Cheers

nanometrenat avatar Feb 19 '24 18:02 nanometrenat

@frozenhelium @batpad @tovari did this get addressed in the GO rewrite or is it still an open thing for build? Cheers

Checking in! Is action still required under this?

nanometrenat avatar May 17 '24 11:05 nanometrenat

@nanometrenat this issue has been addressed. Can be closed!

frozenhelium avatar May 17 '24 13:05 frozenhelium