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

Add in ability to edit pins #95

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
4 changes: 2 additions & 2 deletions App.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import NavigationService from './utils/NavigationService';
import Onboarding from './containers/Onboarding';
import Navigation from './nav';
import reducer from './reducers';
import { createDatabases } from './apis/database';
import { createOrUpdateDatabase } from './apis/database';
import { initialize } from './apis';
import { restore } from './actions/authentication';
import { setup } from './actions/connectivity';
Expand Down Expand Up @@ -45,7 +45,7 @@ class App extends Component {
componentDidMount() {
Promise.all([
...this.loadFontsAsync(),
createDatabases(),
createOrUpdateDatabase(),
I18n.initAsync(),
store.dispatch(positionActions.initialize()),
store.dispatch(
Expand Down
110 changes: 94 additions & 16 deletions actions/locations.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import Sentry from 'sentry-expo';
import { Alert } from 'react-native';
import i18n from '../assets/i18n/i18n';
import * as apis from '../apis';
import { requestPushPermission } from './permissions';
import { containing } from './regions';
Expand All @@ -15,6 +17,14 @@ function receiveLocation(location) {
};
}

export const REMOVE_LOCATION = 'REMOVE_LOCATION';
function removeLocation(locationKey) {
return {
type: REMOVE_LOCATION,
locationKey,
};
}

export const CLEAR_LOCATIONS = 'CLEAR_LOCATIONS';
export function clearLocations() {
return {
Expand All @@ -40,6 +50,8 @@ function wrapper(work, location) {
await work;
await dispatch(updateUploadStatus(location, true));
dispatch(uploaded(location.key));
const uploadedLocation = { ...location, uploaded: true };
dispatch(receiveLocation(uploadedLocation));
} catch (err) {
Sentry.captureException(err, {
extra: {
Expand Down Expand Up @@ -69,6 +81,34 @@ export function restoreLocalLocations() {
};
}

export function deleteLocalLocation(locationKey) {
return async (dispatch) => {
const result = await new Promise((resolve) => {
Alert.alert(
i18n.t('location/delete'),
i18n.t('location/confirmDelete'),
[
{ text: i18n.t('button/cancel'), style: 'cancel', onPress: () => resolve(false) },
{ text: i18n.t('button/delete'), onPress: () => resolve(true) },
],
{
cancelable: true,
onDismiss: () => resolve(false),
}
);
});

if (result) {
database
.deleteLocation(locationKey)
.then(() => dispatch(removeLocation(locationKey)))
.catch((err) => {
Sentry.captureException(err);
});
}
};
}

export function fetchLocation(locationKey) {
return async (dispatch, getState) => {
const {
Expand Down Expand Up @@ -96,28 +136,21 @@ export function fetchAllLocationData(locationKey) {
};
}

export function createLocation(options) {
const { latitude, longitude, resources, status } = options;
function pushLocation(localLocation, locationData, apiCall) {
const showOutdatedAlert = () =>
Alert.alert(i18n.t('location/errorUpdate'), i18n.t('location/errorUpdateDescription'), [
{
text: i18n.t('button/ok'),
onPress: () => {},
},
]);

return async (dispatch, getState) => {
const {
authentication: { regionKey: hasRegion },
connected,
onboarding: { hasAddedLocation },
} = getState();

if (!hasAddedLocation) {
dispatch(setCompleted(COMPLETED_KEYS.hasAddedLocation));
}

const locationData = {
latitude,
longitude,
resources,
status,
};

const localLocation = await database.addLocalLocation(locationData);
const { key } = localLocation;

dispatch(offline(key));
Expand All @@ -134,13 +167,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, showOutdatedAlert);

await dispatch(wrapper(saved, location));
}
};
}

export function updateLocation(options, oldLocationKey) {
const { latitude, longitude, resources, status } = options;

return async (dispatch, getState) => {
const { locations } = getState();

const oldLocation = locations[oldLocationKey];

const locationData = {
latitude,
longitude,
resources,
status,
};

const localLocation = await database.updateLocalLocation(locationData, oldLocation);

dispatch(pushLocation(localLocation, locationData, apis.updateLocation));
};
}

export function createLocation(options) {
const { latitude, longitude, resources, status } = options;

return async (dispatch, getState) => {
const {
onboarding: { hasAddedLocation },
} = getState();

if (!hasAddedLocation) {
dispatch(setCompleted(COMPLETED_KEYS.hasAddedLocation));
}

const locationData = {
latitude,
longitude,
resources,
status,
};

const localLocation = await database.addLocalLocation(locationData);
dispatch(pushLocation(localLocation, locationData, apis.createLocation));
};
}

export function pushLocalLocations() {
return async (dispatch, getState) => {
const {
Expand Down
77 changes: 67 additions & 10 deletions apis/database.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { SQLite } from 'expo';
import { SQLite, SecureStore } from 'expo';
import Sentry from 'sentry-expo';
import moment from 'moment';
import { pushRef } from './index';
import { convertArrayToLocations, convertToLocation, createLocationObject, saveCoordinates } from '../utils/database';

const DATABASE_VERSION_KEY = 'DATABASE_VERSION_KEY';
const DATABASE_VERSION = '2';

export function openDatabase(databaseName = 'locations.1.db') {
return SQLite.openDatabase(databaseName);
}
Expand All @@ -23,30 +26,58 @@ export function executeTransaction(statement, args = null) {
});
}

export function createDatabases() {
function createDatabases() {
return executeTransaction(
'create table if not exists locations (id integer primary key not null, key text, coordinateKey text, createdAt text, resources text, status text, uploaded int)'
'create table if not exists locations (id integer primary key not null, key text, coordinateKey text, createdAt text, resources text, status text, uploaded int, updated int)'
);
}

export function clearDatabase() {
return executeTransaction('drop table locations');
}

export function deleteLocation(key) {
return executeTransaction('delete from locations where key = ?', [key]);
}

export function updateUploadStatus(key, isUploaded) {
return executeTransaction('update locations set uploaded = ? where key = ?', [isUploaded, key]);
}

export function updateLocalLocation(options) {
const { resources, status, key } = options;
// Uncomment this if user is ever able to change location position
// SecureStore.setItemAsync(key, JSON.parse({ longitude, latitude }));
export function replaceLocalLocationWithRemote(remoteLocation) {
const { key, latitude, longitude, updated, status, resources } = remoteLocation;

saveCoordinates(key, latitude, longitude);

return executeTransaction('update locations set status = ?, resources = ?, updated = ?, uploaded = 1 where key = ?', [
status,
JSON.stringify(resources),
updated,
key,
]);
}

export async function updateLocalLocation(options, oldLocation) {
const { key, updated = 0 } = oldLocation;
const { latitude, longitude, resources, status } = options;

saveCoordinates(key, latitude, longitude);

const locationObject = createLocationObject(key, {
...oldLocation,
...options,
uploaded: false,
updated: oldLocation.uploaded ? oldLocation.updated + 1 : oldLocation.updated,
});

return executeTransaction('update locations set resources = ?, status = ?, uploaded = 0 where key = ?', [
await executeTransaction('update locations set resources = ?, status = ?, uploaded = 0, updated = ? where key = ?', [
Copy link
Owner

Choose a reason for hiding this comment

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

Does this also need a transaction or to enforce that update is equal to update - 1 before it is being set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But I already do a check to ensure the remote data hasn't been updated more recently no?

Copy link
Owner

@smaclell smaclell Jan 20, 2019

Choose a reason for hiding this comment

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

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.

resources,
status,
updated + 1,
key,
]);

return locationObject;
}

// fetches individual location
Expand Down Expand Up @@ -80,9 +111,35 @@ export async function addLocalLocation(locationData) {

const createdAt = moment.utc(locationObject.created).toISOString();
await executeTransaction(
'insert into locations (key, coordinateKey, createdAt, resources, status, uploaded) values (?, ?, ?, ?, ?, ?)',
[key, key, createdAt, resourcesString, status, 0]
'insert into locations (key, coordinateKey, createdAt, resources, status, uploaded, updated) values (?, ?, ?, ?, ?, ?, ?)',
[key, key, createdAt, resourcesString, status, 0, 0]
);

return locationObject;
}

export async function createOrUpdateDatabase() {
const version = await SecureStore.getItemAsync(DATABASE_VERSION_KEY);

await createDatabases();
smaclell marked this conversation as resolved.
Show resolved Hide resolved

if (version === DATABASE_VERSION) {
return Promise.resolve();
smaclell marked this conversation as resolved.
Show resolved Hide resolved
}

const locations = await fetchLocalLocations();

await clearDatabase();
Copy link
Owner

Choose a reason for hiding this comment

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

This feels extremely dangerous. If something went wrong we would lose all the data ... I wonder if we could do something side by side.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose we could create a temporary database and store all the locations we pulled out of the old one and then delete it after we have populated the new one again. How would you feel about that?

Copy link
Owner

Choose a reason for hiding this comment

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

I think that would be fine. Simpler would be to have a different table to back things up into. You might be able to do it in a single query too.

await createDatabases();

const transactions = locations.map((location) => {
const { key, createdAt, resources, status, uploaded = 0, updated = 0 } = location;

return executeTransaction(
smaclell marked this conversation as resolved.
Show resolved Hide resolved
'insert into locations (key, coordinateKey, createdAt, resources, status, uploaded, updated) values (?, ?, ?, ?, ?, ?, ?)',
[key, key, createdAt, JSON.stringify(resources), status, uploaded, updated]
);
});

return Promise.all(transactions).then(() => SecureStore.setItemAsync(DATABASE_VERSION_KEY, DATABASE_VERSION));
}
41 changes: 40 additions & 1 deletion apis/index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import Sentry from 'sentry-expo';
import Expo from 'expo';
import * as firebase from 'firebase';
import GeoFire from 'geofire';
import { wrapLatitude, wrapLongitude } from '../utils/geo';
import { createLocationObject } from '../utils/database';
import { replaceLocalLocationWithRemote } from '../apis/database';

export function initialize() {
if (firebase.initialized) {
Expand Down Expand Up @@ -86,7 +88,44 @@ export function fetchLocation(locationKey) {
.then((location) => location.val());
}

export async function createLocation(regionKey, options, key) {
export async function updateLocation(regionKey, key, newLocation, showOutdatedAlert) {
initialize();

let pushed;
if (key) {
pushed = getRef(`locations/${key}`);
} else {
pushed = pushRef('locations');
}

try {
const existing = await fetchLocation(key);
smaclell marked this conversation as resolved.
Show resolved Hide resolved
if (existing && existing.key && existing.updated >= newLocation.updated) {
showOutdatedAlert();
smaclell marked this conversation as resolved.
Show resolved Hide resolved
await replaceLocalLocationWithRemote(existing);
return {
created: existing,
saved: Promise.resolve(),
};
}
} catch (err) {
Sentry.captureException(err, {
extra: {
locationKey: key,
},
});
}

const saved = pushed.set(newLocation);
const geoPromises = saveGeoData(newLocation, pushed.key, regionKey);

return {
created: newLocation,
saved: Promise.all([saved, ...geoPromises]),
};
}

export async function createLocation(regionKey, key, options) {
initialize();

let pushed;
Expand Down
7 changes: 7 additions & 0 deletions assets/i18n/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"button/push_locations": "Upload",
"button/save": "Save",
"button/offline": "Offline",
"button/delete": "Delete",
"connectivity/action_requires_connection": "The requested action requires an active network connection",
"export/body": "Here is my data from the Share Bibles App",
"export/error_title": "Export Failed",
Expand All @@ -17,6 +18,11 @@
"invites/error_title": "Access Code Issue",
"invites/error": "There was an issue accepting your Access Code, please try again and/or contact {{email}}",
"invites/placeholder": "Access Code",
"location/edit": "Edit location",
"location/delete": "Delete location",
"location/errorUpdate": "Edit failed",
"location/errorUpdateDescription": "Someone else already updated this location. We have downloaded their update to your device. Please review the update and try your changes again.",
"location/confirmDelete": "Are you sure you want to delete this location? This action cannot be undone!",
"locationData/failed_upload": "Failed to upload: {{value}}",
"locationData/incorrect_region": "Location is not within your current region!",
"locationData/latitude": "Latitude: {{value}}",
Expand Down Expand Up @@ -85,6 +91,7 @@
"title/accept_invites": "Accept Access Codes",
"title/location_data": "Upload Status",
"title/share_bibles": "Share Bibles",
"title/editing": "Editing",
"validation/no_status_message": "You must provide a status",
"validation/no_status_title": "No status",
"validation/unknown_error_message": "An unexpected error has occurred, please try again.",
Expand Down
Loading