-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
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 |
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! |
------- 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.