Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/mutipleareas #34

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Feature/mutipleareas #34

wants to merge 7 commits into from

Conversation

jacko529
Copy link
Collaborator

@jacko529 jacko529 commented Feb 5, 2020

------- Issues to change

-- Downloading tile error
change in file download.service.js
the problem being in function downloadAndZipCoordinates()
due to using promises.all if one of the requests fails, they all fall over.
I suggest to map the requests which fail to a new promise array, then we try and request the error promise again. If it fails again, just set it to null. Thereby returning something rather than failing all-together.

-- Grab areas with template id
I have created a new endpoint, /find/:id. This is applied in getAOIs, it grabs the area ID's which contain the template ID's from is requested form the form microservice.

-- image fallback
A image placeholder was provided it cases the snapshot cannot be taken.

-- multiple areas of interest
Most of the work here is in the update function. When the request comes with the property 'override' the model will update accordingly overriding the the templateIDs array with the new area of interest.

If it a template with no area of interest then it will just add to the area like normal.

-- model change -- backward compatibility
IMPORTANT

The issue we will have is backward compatibility with the fields, the model has been updated with templateID : string (this is the original field) and templateIDs : array (this is the new field). All new frontends will utilise templateIDs, however we have to keep templateID as older devices who have not updated will require the old property. This is more for read purposes the write purposes, when writing it will write to both properties however the actual value in the old property is not important and will just pick one form the array.

@jacko529 jacko529 requested a review from tiagojsag February 5, 2020 15:50
@tiagojsag tiagojsag self-assigned this Feb 6, 2020
Copy link
Contributor

@tiagojsag tiagojsag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

If it's ok, I'd like to see some top-level changes before diving into reviewing the actual functional changes, so we can have this PR follow the best practices we've been applying to the rest of the API code changes in the past few months:

  • Existing tests seem to be failing on this branch, but passing on the target branch.
  • It would be great if you could add tests to cover the existing endpoint you're modifying, so we minimize the risk of an undesired change in behavior
  • New endpoint added doesn't seem to be covered by any test
  • I see a few code style violations, please use the built-in eslint CLI tool and ruleset
  • Please reflect the new behavior implemented here in the API docs: https://github.com/resource-watch/doc-api
  • I see a hardcoded URLs, please use an ENV vars instead

Let me know your thoughts on this
Thanks

@tiagojsag tiagojsag assigned jacko529 and unassigned tiagojsag Feb 6, 2020
@jacko529
Copy link
Collaborator Author

jacko529 commented Feb 6, 2020

  • if

Thank you for replying so quickly. I was unfamiliar was the required needs of the application however I testing has been implemented on this change. Any more issues please let me know, the update to the API doc will come next.

Jack

@tiagojsag
Copy link
Contributor

Hi @jacko529 sorry for leaving this parked for so long, but other priorities came up. Before I do an in-depth CR, could I ask you to rebase your branch with the latest changes done in develop? thanks!

@tiagojsag tiagojsag changed the base branch from develop to dev October 14, 2020 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants