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

Create db cleanups for models #34

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Create db cleanups for models #34

wants to merge 8 commits into from

Conversation

GoodluckH
Copy link
Contributor

@GoodluckH GoodluckH commented Jul 23, 2022


@GoodluckH GoodluckH requested a review from swansontec July 23, 2022 00:37
@GoodluckH
Copy link
Contributor Author

Before proceeding to other models, please take a look if the
changes were as intended.

Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

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

We need to get this tested. I have set up a push-test.edge.app server, so we can just deploy the code there and then tweak the edge-react-gui app to talk there instead of the production server. For that to work, we need to ensure that the database setup code also sets up the correct view documents. Otherwise the queries will fail.

Comment on lines 11 to 13
const cleanedDoc = uncleaner(cleaner)(doc)
await db.insert({
...cleanedDoc.doc,
_id: cleanedDoc.id,
_rev: cleanedDoc.rev ?? undefined
})
Copy link
Contributor

Choose a reason for hiding this comment

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

The asCouchDoc uncleaner is supposed to handle this stuff. So db.insert(wasCouchThing(packThing(data))) is all you really need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still not so sure how this works since insert expect a nano document type but asCouchDoc cannot be dereferenced to nano. For example, although they are structurally similar, CouchDoc names id as 'id' rather than '_id'.

Copy link
Contributor

Choose a reason for hiding this comment

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

An uncleaner undoes the effect of a cleaner. That means re-assembling the fields the way Couch likes:

const asCouchFoo = asCouchDoc(asObject({ foo: asString }))
const wasCouchFoo = uncleaner(asCouchFoo)

asCouchFoo({ _id: '1', _rev: '2', foo: '3' }) // Goes to the clean format
wasCouchFoo({ id: '1', rev: '2', doc: { foo: '3' } }) // Goes back to the couch format

@GoodluckH GoodluckH requested a review from swansontec July 25, 2022 22:58
Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

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

We are getting a lot closer! Time for round two. Sorry if this is annoying, but that's why this work takes a long time - you have to do enough cleanup before you can even see the next level of dirtiness.

Another general note is that I have always been putting connection as the first parameter, not the last one. That keeps things consistent, since it's always present in the same place. I like to leave the last slot open for optional parameters.

I have taken the API key and defaults database cleanups into the serverlet PR, so we can merge them right away. That way your PR can focus just on the, users, devices, and thresholds.

Comment on lines 11 to 13
const cleanedDoc = uncleaner(cleaner)(doc)
await db.insert({
...cleanedDoc.doc,
_id: cleanedDoc.id,
_rev: cleanedDoc.rev ?? undefined
})
Copy link
Contributor

Choose a reason for hiding this comment

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

An uncleaner undoes the effect of a cleaner. That means re-assembling the fields the way Couch likes:

const asCouchFoo = asCouchDoc(asObject({ foo: asString }))
const wasCouchFoo = uncleaner(asCouchFoo)

asCouchFoo({ _id: '1', _rev: '2', foo: '3' }) // Goes to the clean format
wasCouchFoo({ id: '1', rev: '2', doc: { foo: '3' } }) // Goes back to the couch format

Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

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

Ah, this is much nicer! One question this time.

@GoodluckH GoodluckH closed this Jul 28, 2022
@GoodluckH GoodluckH reopened this Jul 28, 2022
@GoodluckH GoodluckH force-pushed the xipu/db-cleanup branch 4 times, most recently from fd54b08 to a8806c1 Compare July 29, 2022 00:43
@GoodluckH GoodluckH requested a review from swansontec July 29, 2022 00:44
Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

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

This looks almost ready. We need to fix some conflict errors, and then we can deploy to push-test.edge.app for testing.

@GoodluckH GoodluckH requested a review from swansontec July 29, 2022 20:43
@GoodluckH GoodluckH marked this pull request as ready for review July 29, 2022 20:43
@swansontec
Copy link
Contributor

I have deployed this branch to the push-test.edge.app server, but it's not working very well. If you want to test it yourself, I can get you credentials to work with the server. To adjust the client, just modify edgeConfig.js in edge-react-gui to point to notificationServers: ['https://push-test.edge.app']

@swansontec swansontec mentioned this pull request Aug 17, 2022
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