openkfw / Oscar

A digital decision support system for humanitarian operations
https://www.oscarplatform.org/
GNU General Public License v3.0
11 stars 1 forks source link

Get map layers with or without geodata #77

Closed srezacova closed 2 years ago

srezacova commented 2 years ago

Description

Refactor getMapLayers function to return map layers with or without geoData.

How has this been tested?

  1. Create initial-data-load/data/testCountry4 folder
  2. Copy initial-data-load/data/Sample/MapLayers.yml to testCountry4 folder
  3. Remove geoReferenceId from layers config
  4. Add COUNTRY=testCountry4 to .env file
  5. Run ./start.sh
  6. Run ./runinitialload.sh
  7. In UI map layers click on Select layers
  8. You can see layers there, click on some layer
  9. This will cause TypeError. This should never happen because this functionality is not supposed for mapLayers we have there now. These mapLayers should have geoReferenceId always.
  10. Check the same for Sample, to see if layers with geoData are uploaded properly

Screenshots (if appropriate):

Types of changes

Checklist:

srezacova commented 2 years ago

Hi, I have just few questions.

  1. getGeojsonData function in vectorSourceLoader and boxVectorSourceLoader return exactly the same structure? I just have some problem with debugging on frontend so I cannot show it in console. In case of boxVectorSourceLoader it is geojsonData that we have in pointAttributes collection documents?

  2. Error I mentioned in point 9 is:

    Unhandled Rejection (TypeError): Cannot read properties of undefined (reading 'protocol')
    
    92 | export const getGeojsonData = async (url) => {
    > 93 |   const response = await axios.get(url);
    94 |   return response.data;
    95 | };

    I can get rid of it by adding something like this:

      if (url) {
        response = await getGeojsonData(url);
      }

    but I am not sure if it is necessary. Then I need to also put handleIsLoading(title, 'remove'); to LN76. And in points.js on LN11 there is also condition that depends on geoReferenceId, that I must change if I do these changes.

  3. In the first test when I will try to expect something like this: expect(res.body[0].layerType).toEqual(mapLayerWithoutGeodata.layerType); It will not work, it uses MapLayer model which uses MapLayerSchema which do not include layerType. But when we are getting data with getMapLayers function, we use exactly the same MapLayer model and it return all properties we have in database layerType included. Whats the reason? Should I use SingleMapLayer discriminator instead?

Thanks for answer

bariela commented 2 years ago

@srezacova hi,

  1. it is literally the same function, so definitely yes. Difference might be only in code of the respective loaders
  2. sounds reasonable, but also with else (returning probably just [] ?)
  3. definitely the discriminator, the model itself is just some common parts for both of them
srezacova commented 2 years ago

@srezacova hi,

  1. it is literally the same function, so definitely yes. Difference might be only in code of the respective loaders
  2. sounds reasonable, but also with else (returning probably just [] ?)
  3. definitely the discriminator, the model itself is just some common parts for both of them
  1. I fixed this in VectorSourceLoader. You can test it with layer with no geoReferenceId.In case that normal mapLayers wouldn't have in some case geoReferenceId, what to do with condition on LN10 in points.js? Now it uses Kobo loader for these layers. Will this case ever happen for normal mapLayers? If not I will leave it like this.
  2. I put discriminator in first test, but it will not work for second test with:
        color: {
          type: 'colormap',
          value: 'green',
        },

    in legend, because SingleMapLayer is expecting color as string. Thats probably the reason why we used MapLayer model there? getMapLayers function in mapLayersModel.js return are all map layers properties even those that are not in MapLayer model. (for example layerType) Why? It should only return those properties that are in MapLayerSchema or not? Thanks for answer

bariela commented 2 years ago

@srezacova hi,

  1. it is literally the same function, so definitely yes. Difference might be only in code of the respective loaders
  2. sounds reasonable, but also with else (returning probably just [] ?)
  3. definitely the discriminator, the model itself is just some common parts for both of them
  1. I fixed this in VectorSourceLoader. You can test it with layer with no geoReferenceId.In case that normal mapLayers wouldn't have in some case geoReferenceId, what to do with condition on LN10 in points.js? Now it uses Kobo loader for these layers. Will this case ever happen for normal mapLayers? If not I will leave it like this.
  2. I put discriminator in first test, but it will not work for second test with:
        color: {
          type: 'colormap',
          value: 'green',
        },

in legend, because SingleMapLayer is expecting color as string. Thats probably the reason why we used MapLayer model there? getMapLayers function in mapLayersModel.js return are all map layers properties even those that are not in MapLayer model. (for example layerType) Why? It should only return those properties that are in MapLayerSchema or not? Thanks for answer

Hi,

  1. I would resolve already in axiosRequests.js, so the loader gets just correct data format. Maybe even try catch and finally? So error in one data won't break the map
  2. color as string is correct for all models, type is on the same level, it is slightly different format from style. It is filtered at creating/saving to db. That is the point of having models. For getMapLayers we use MapLayer model to get both single and group map layers = layers from all discriminators

I realised just now one new issue - we should not return mapLayers of layerType='regions' with no geoData. Using the old filter & adding condition for layerType 'points' here https://github.com/openkfw/Oscar/pull/77/files#diff-4401c75e5a0122ab94518d56931c9b5eb06c7a1af2764942c5c4b7d050dd9750L18 could work with this requirement.