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

Update example app and demonstrate some other best practices #239

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

Conversation

rjcorwin
Copy link

@rjcorwin rjcorwin commented Jun 28, 2017

I was having some issues getting the example todo app running and @vinz243 had a good request in #224 that on application first start it set up some security.

  • Add a package.json so modules are available.
  • Add a README with todos installation and start
  • Disable admin party by creating an admin
  • Setup todo database on app start
  • Setup validation
  • Setup users
  • Setup permissions

@rjcorwin
Copy link
Author

Getting closer. It now creates an admin and then creates an app database using those admin credentials.

@rjcorwin rjcorwin changed the title Boilerplate application server example Update example app and demonstrate some other best practices Jun 28, 2017
@rjcorwin
Copy link
Author

application-server-boilerplate folder is now merged into todos folder

@rjcorwin
Copy link
Author

@nolanlawson I may not get to the other items on my checklist so it may be good as is. I could squash the commits in this PR if you like.

Copy link
Member

@marten-de-vries marten-de-vries left a comment

Choose a reason for hiding this comment

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

Hi! Sorry for only getting to this now. It looks really nice! I gave two inline comments with suggestions, but if you disagree I'm fine with merging this as-is, at it is helpful like this too. I can squash the commits together when merging, but if you want to do so using a rebase that is fine as well.


async function setup() {

// Set up the admin user.
Copy link
Member

Choose a reason for hiding this comment

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

app.couchConfig.set('admins', config.admin.username, config.admin.password, function (err, previousValue) {})

might be more robust. Less chance, although that chance is already small admittedly, of someone hijacking your database before the admin is set. Note that I did not run the code, so using it would require testing it.

Copy link
Author

Choose a reason for hiding this comment

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

Less chance, although that chance is already small admittedly, of someone hijacking your database before the admin is set.

Lol, ya, I've thought about that but not overly worried. I didn't know about app.couchConfig.set. Cool stuff!

Copy link
Contributor

@QuentinRoy QuentinRoy Aug 10, 2018

Choose a reason for hiding this comment

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

app.couchConfig.set does not seem to work. It makes it crash with "TypeError: refreshUsersDBImpl is not a function". What is weird is that it still set up the config file properly...

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems what's happening in this case is that the users data base isn't ready yet, but setting up an admin will still cause it to try to update it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've created an issue here #341

console.log("We already have admins.");
}

// Set up the app database.
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, if you store the PouchDB.defaults() intermediate result above, you could just do new PouchDB('todos') here. Again, it removes the HTTP roundtrip.

@rjcorwin
Copy link
Author

rjcorwin commented Feb 1, 2018

I'm in the middle of a crunch right no so I might not get to this for a couple of weeks but thank you so much for reviewing!!

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.

3 participants