Add in ability to edit pins #95
Conversation
|
actions/locations.js
Outdated
| @@ -134,13 +159,58 @@ export function createLocation(options) { | |||
| return; | |||
| } | |||
|
|
|||
| const { created: location, saved } = await apis.createLocation(regionKey, locationData, key); | |||
| const { created: location, saved } = await apiCall(regionKey, key, localLocation); | |||
There was a problem hiding this comment.
locationData to localLocation. I cannot tell is that is a bad thing. It sure is different.
There was a problem hiding this comment.
This is done on purpose. The data is updated or created in the local database first and the result of that is then used to send to the server since the localLocation object contains info like the location version number, where as the locationData object does not.
There was a problem hiding this comment.
How does versioning/changes work if it is only local?
There was a problem hiding this comment.
Could you be a little more specific? When sending to the remote we do store the version info. It's just that the locationData object here doesn't necessarily include that information.
There was a problem hiding this comment.
I was expecting the version number to be remote since we need to make sure data is synced correctly in firebase.
Your comment cleared it up a bit. So we do send it, just not necessarily initially.
apis/database.js
Outdated
| updated: oldLocation.uploaded ? oldLocation.updated + 1 : oldLocation.updated, | ||
| }); | ||
|
|
||
| await executeTransaction('update locations set resources = ?, status = ?, uploaded = 0, updated = ? where key = ?', [ |
There was a problem hiding this comment.
Does this also need a transaction or to enforce that update is equal to update - 1 before it is being set?
There was a problem hiding this comment.
I'm not sure that's totally necessary. The value of updated will always be less than the new value. If it is less than the new value -1 then we are just making an even newer update to the local location and it should be a non-issue. Is there something else I am overlooking?
There was a problem hiding this comment.
I think you might be okay with the local version, but you might not be able to guarantee that you are the only person to write to the remote data.
There was a problem hiding this comment.
But I already do a check to ensure the remote data hasn't been updated more recently no?
There was a problem hiding this comment.
You do, but there is still a race condition. Multiple people could try to update it at the same time which would still result in one overwriting the other if the concurrency bit is only set locally. I was expecting something to be passed up with the update to prevent races in firebase.
We can setup a remote rule for firebase to prevent updates if the version number is not expected.
screens/overviewMap.js
Outdated
| deleteLocalLocation(pinLocation.key); | ||
| } else { | ||
| // Is there a better way to get the pin to rerender??? Cause this is actually gross... | ||
| this.setState(() => ({ editingPinKey: pinLocation.key }), () => this.setState(() => ({ editingPinKey: null }))); |
There was a problem hiding this comment.
I think you might be able to call a method to force it to rerender. You are right. This is gross.
There was a problem hiding this comment.
Apparently, there is something weird with markers and they only fully redraw when their key prop changes so I had to use a Date.now() to build a unique key every time the LocationMarker component rerenders. But I think that's still better than the old way.
There was a problem hiding this comment.
Oh interesting? Are they pure or have a shouldComponentUpdate?
There was a problem hiding this comment.
As far as I can see it's not pure and doesn't have a shouldComponentUpdate
There was a problem hiding this comment.
Odd. Must be something from the maps library. I think I saw one of the items higher in the hierarchy was pure, but that could just be a red herring.
There was a problem hiding this comment.
Mind adding a comment as to why we are doing this odd things?
screens/overviewMap.js
Outdated
| screenHeight = Dimensions.get('window').height * (Platform.OS === 'ios' ? 1 : Dimensions.get('window').scale); | ||
|
|
||
| createTempPin = (coord, editingPinKey) => { | ||
| this.setState(() => ({ tempLocation: coord, editingPinKey })); |
There was a problem hiding this comment.
Style: I have since learned it is better to use the following. It updates just those keys whereas what you are doing will overwrite alot more.
| this.setState(() => ({ tempLocation: coord, editingPinKey })); | |
| this.setState({ tempLocation: coord, editingPinKey }); |
|
I like the back up and think you are well on your way. Let's sync up on hangouts to see what is left to get this one out. I think it might be ready. |
|
I have performed a basic round of testing. It has filled me with more respect/fear for this feature. Added a few more things to test to my list. May try to think of simpler ways we can roll this out. |
smaclell
dismissed
their stale review
Since I have been adding fixes/improvements I don't think I my previous review applies.
|
@jfangrad what stops a baddy from editing all the pins they want? Could one bad actor wipe out all our data? |
This PR adds in the feature to edit/delete locations.
Also adds in database update migration. (
createOrUpdateDatabasefunction in database.js)To edit/delete a pin, simply press and hold and drag the location to one of the options and then release it.
Deleting is only available if the location has never been uploaded to the database. (It will only delete the local version anyways, it doesn't even attempt to delete anything on the server side).
Editing is always available. Location version details are now kept with each location. If you attempt to edit a location that has a newer version on the server you will receive an error that states that the edit failed due to a newer version existing on the server. It will then update that location and inform the user that they can now try again editing the latest version.
Todo: