start #19

[ransomw]

this is a start on issue #19

the (very preliminary) test fails (as expected). later on, i'll commit more tests to this branch and then an implementation.

---

closes #19

[ransomw]

yep. i'll ping back when there are tests for review (estimate: this weekend or early next week).

when i get around to the implementation, let's try to get the s/bind\(this/bind(null/g edit in as part of this pr (?).

[gr2m]

when i get around to the implementation, let's try to get the s/bind(this/bind(null/g edit in as part of this pr (?).

let’s make a separate one for this, after #19 is fixed

[ransomw]

there are some questions about tests in comment lines beginning with ???, and some questions that i expect to arise later on in comment lines marked with (?). implementation even though it's not committed to this branch. feedback on any or all the things is appreciated.

i suppose the next step toward closing this pr/issue is getting a go-ahead on the tests so that i can merge in the implementation... dunno how to comment on diffs... seems i need to do separate comments for that...

[gr2m]

Hey @ransomw, I’m sorry for not getting back to you on this, I’ll look at your work asap

[ransomw]

hey @gr2m, no need to apologize: buried in koa and generators and not in any great rush on my end :semi-relaxed:

[gr2m]

I think I gave feedback to your open questions? Anything else I can help you with? Great work so far, thanks Ransom!

[ransomw]

yep, many thanks: this is what i needed to finish tests. i'll tentatively plan on finishing them by next weekend if nothing else comes up.

[ransomw]

sokay, here are the tests, to be read as a proposal for spec coverage based on the README and comments both in this thread and #19 ..

the current implementation (unmerged) passes all tests, except one (will add line comment).

also, note that i've updated the suggested fixture from #19 to have an includes array b/c the README specifies that account always be defined.

[ransomw]

alright, sorry for the delay. anticipated the ConnectionError task taking more time for some reason..

went ahead and merged in implementation and README updates, so all the things are ready for review.

[gr2m]

great work so far, left a few comments, but thanks for the amazing contribution! Looking forward to merge it in!

[ransomw]

so i made the changes for 2/3 comments. (case convention mismatch :flushed:).

for the accounts.find/findAll thing, i suppose it's not so much a question of what to include as what what in include as part of this pr. i was kind of thinking that after this got closed, separate prs could do the this/null change, more complete spec coverage in tests, and the findAll/find edit, among other things. in particular, usage of find/findAll and the corresponding tests for sessions.add would be clearer to me if there were tests for these functions.

if the findAll/find edit needs to happen for this pr, i can give it a try. let me know what you think.

[gr2m]

okay I remember now, the problem with using accounts.find is that we don’t know the account.id and our REST API does not support to find an account by username, which is what we would need for admin.sessions.add({username: 'pat'}).

Using admin.accounts.findAll() is a bit wastful, imagine there would be thousands of accounts, we would fetch them all, then filter locally.

Probably the best would be to support the ?filter query parameter as described in the spec. So something like GET /accounts?filter={"username":"pat"} could work it would return either 1 or no results.

Okay I just left a last few minor comments, then this is good to merge and we can address the rest in follow up issues

[ransomw]

oh yea, i was blanking on what i'd meant by that TODO myself.

changes to the commented comments are merged. #82 is open for find/findAll and #83 for bind(null .

the new issues were typed in a rush. i can edit them more later.

[gr2m]

this looks good @ransomw :+1:

Before we can merge, we need to clean up the git history. I’m happy to help you with that, just invite me as collaborator to your fork https://github.com/ransomw/hoodie-client-account/settings/collaboration

If you want to give this a go yourself, here is what you need to do:

1. Rebase on the latest master branch

git remote add upstream https://github.com/hoodiehq/hoodie-client-account
git fetch upstream
git checkout -b upstream-master upstream/master
git pull upstream master
git checkout iss19-pr01
git rebase upstream-master

1. reset your commits

# a0310cc is the commit hash of the last commit in master
git reset a0310cc
# now commit your changes following our commit conventions
git commit package.json -m "chore(package): nock@^7.2.2"
git commit admin/README.md -m "docs(admin): remove obsolete link to issue"
git add test
git commit -m "test: accountAdmin.sessions.add"
git add .
git commit -m "feat: accountAdmin.sessions.add"

1. update your PR

git push -f origin iss19-pr01

Let me know if that all makes sense. I’m happy to explain, this was all very confusing to me only a while ago :)

[ransomw]

history has been rewritten..

are commit message conventions documented? particularly, what's an appropriate commit message for adding a *~ (emacs backup) line to .gitignore?

[gr2m]

We follow Angular’s commit conventions: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#-git-commit-guidelines

For .gitignore file, we usually do chore(gitignore): *~ when adding *~ to it.

Thanks for all the hard work, this is good to merge now, LGTM

ping @hoodiehq/maintainers for 2nd review :eyes:

[HipsterBrown]

LGTM! :+1:

Wonderful work @ransomw! Merging asap.

[HipsterBrown]

Released as https://github.com/hoodiehq/hoodie-client-account/releases/tag/v2.8.0! :clap: