This includes at least the Google auth object.

We're making the form ID column in the database a VARCHAR, so we need to determine a maximum length. What is an appropriate maximum? (Is there a maximum?)

The answer might be "Other: inline-documentation sucks, do this instead."

But especially since JS is dynamically typed, there needs to be an easy API function signature reference of some kind.

Clint writes:

Tune the linter according to the notes I had on the Airbnb lint profile, and resolve any outstanding issues that then remain.

To match the js so we're not converting all over the place.

From our call today, it sounds like we want to add integration tests in addition to unit tests for the database. This issue is to add those integration tests: #10 encompasses only unit tests.

As part of this, I think we should create a test database?

Probably yes because the way submissions work is by formid, so we don't know how to target otherwise.

And not just the final component. (eg meta.instanceID rather than just instanceID)

We'll do this once we decide on the app's name.

Right now, in some cases, if a request causes the server to throw an error, the stack trace is sent back as the response. How about we add error handling such that such cases result in a 500 response as JSON rather than a stack trace? I think that even if the JavaScript has a syntax error, tries to reassign a const, etc., that shouldn't result in a stack trace sent to the end user.

There are some fundamental issues with it:

1. It does not accept streams as input.
2. It swallows text nodes when they are siblings to child nodes.
3. It presents attributes differently (in a way our code currently does not handle well) when there are vs aren't child nodes.

If container is created at top-level scope in test/api/setup.js (eg the injector initialization inside testService is moved above that block), it is the first-created container. Then, the initialized services accidentally use the container generate to run the fixture scripts (which is created last) instead of the container we explicitly pass in in testService. This should not be happening. I suspect it may have something to do with Ramda curry doing unexpected nasty things, but I have bigger fish to fry at the moment.

How about we add an endpoint for GET /forms to list all form definitions?

I noticed this TODO comment in server.js:

https://github.com/nafundi/jubilant-garbanzo/blob/a14a2864335da7379739c340fabbf71d287deb9e/lib/server.js#L114

Do we need to clean up the database after the server shuts down?

Clint writes:

• Devise a testing scheme for … database calls that elude straightforward unit testing.
• Unit test more/all of the surface area we have imported already.

You could also split this into two issues if that's easier…

When running tests:

node -e 'const { withDatabase, migrate } = require("./lib/model/database"); withDatabase(migrate);'
Unhandled rejection Error: connect ECONNREFUSED 127.0.0.1:5432
    at Object._errnoException (util.js:1026:11)
    at _exceptionWithHostPort (util.js:1049:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1174:14)

I think postgres just isn't started.

Form ID + instance ID should uniquely identify submissions. I think we just need to add a uniqueness constraint on the existing index...

Continuing from the commit message in d1cdabb. Should we add linting to CircleCI?

Just need to strip them.

This should be Apache License 2.0, right?

My vote would be to have merge() in util.js match the behavior of Object.assign() and spread syntax: I think any differences could be confusing. Right now, merge() automatically concatenates values that are arrays (rather than replacing them) and will not replace a value with null or undefined. In most cases, I think we'd want merge() to fully replace values. An implementation could maybe we as simple as:

const merge = (...objs) => Object.assign({}, ...objs);

Alternatively, perhaps we could have merge take an options parameter...

Thoughts?

Hi! I've cloned the repo and tried an npm install but it has failed.

• node -v tells v9.3.0
• npm -version tells 5.6.0

This is the output:

> [email protected] install /home/guillermo/src/nafundi/jubilant-garbanzo/node_modules/bcrypt
> node-pre-gyp install --fallback-to-build

node-pre-gyp ERR! Tried to download(404): https://github.com/kelektiv/node.bcrypt.js/releases/download/v1.0.3/bcrypt_lib-v1.0.3-node-v59-linux-x64.tar.gz 
node-pre-gyp ERR! Pre-built binaries not found for [email protected] and [email protected] (node-v59 ABI) (falling back to source compile with node-gyp) 
make: se entra en el directorio '/home/guillermo/src/nafundi/jubilant-garbanzo/node_modules/bcrypt/build'
  CXX(target) Release/obj.target/bcrypt_lib/src/blowfish.o
  CXX(target) Release/obj.target/bcrypt_lib/src/bcrypt.o
../src/bcrypt.cc: In function ‘void encode_salt(char*, u_int8_t*, u_int16_t, u_int8_t)’:
../src/bcrypt.cc:132:1: warning: ‘__builtin___snprintf_chk’ output may be truncated before the last format character [-Wformat-truncation=]
 encode_salt(char *salt, u_int8_t *csalt, u_int16_t clen, u_int8_t logr)
 ^~~~~~~~~~~
In file included from /usr/include/stdio.h:862:0,
                 from ../src/bcrypt.cc:48:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:65:44: note: ‘__builtin___snprintf_chk’ output between 4 and 5 bytes into a destination of size 4
        __bos (__s), __fmt, __va_arg_pack ());
                                            ^
../src/bcrypt.cc: In function ‘void bcrypt(const char*, const char*, char*)’:
../src/bcrypt.cc:165:1: warning: ‘__builtin___snprintf_chk’ output may be truncated before the last format character [-Wformat-truncation=]
 bcrypt(const char *key, const char *salt, char *encrypted)
 ^~~~~~
In file included from /usr/include/stdio.h:862:0,
                 from ../src/bcrypt.cc:48:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:65:44: note: ‘__builtin___snprintf_chk’ output between 4 and 5 bytes into a destination of size 4
        __bos (__s), __fmt, __va_arg_pack ());
                                            ^
  CXX(target) Release/obj.target/bcrypt_lib/src/bcrypt_node.o
In file included from ../../nan/nan.h:192:0,
                 from ../src/bcrypt_node.cc:1:
../../nan/nan_maybe_43_inl.h: In function ‘Nan::Maybe<bool> Nan::ForceSet(v8::Local<v8::Object>, v8::Local<v8::Value>, v8::Local<v8::Value>, v8::PropertyAttribute)’:
../../nan/nan_maybe_43_inl.h:112:73: warning: ‘v8::Maybe<bool> v8::Object::ForceSet(v8::Local<v8::Context>, v8::Local<v8::Value>, v8::Local<v8::Value>, v8::PropertyAttribute)’ is deprecated: Use CreateDataProperty / DefineOwnProperty [-Wdeprecated-declarations]
   return obj->ForceSet(isolate->GetCurrentContext(), key, value, attribs);
                                                                         ^
In file included from /home/guillermo/.node-gyp/9.3.0/include/node/v8.h:26:0,
                 from /home/guillermo/.node-gyp/9.3.0/include/node/node.h:63,
                 from ../../nan/nan.h:49,
                 from ../src/bcrypt_node.cc:1:
/home/guillermo/.node-gyp/9.3.0/include/node/v8.h:3115:29: note: declared here
                 Maybe<bool> ForceSet(Local<Context> context, Local<Value> key,
                             ^
/home/guillermo/.node-gyp/9.3.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
  SOLINK_MODULE(target) Release/obj.target/bcrypt_lib.node
  COPY Release/bcrypt_lib.node
  COPY /home/guillermo/src/nafundi/jubilant-garbanzo/node_modules/bcrypt/lib/binding/bcrypt_lib.node
  TOUCH Release/obj.target/action_after_build.stamp
make: se sale del directorio '/home/guillermo/src/nafundi/jubilant-garbanzo/node_modules/bcrypt/build'
added 522 packages in 5.922s

@clint-tseng and I are thinking that in general, timestamp columns should have the suffix At.

I think we've seen this before, so this may be on @clint-tseng's radar already, but FYI, I'm seeing an error when I run node lib/cli.js user-create:

$ node lib/cli.js -u [email protected] -p password user-create
Required parameter displayName missing.
{ field: 'displayName' }

It was an easy fix, and this isn't blocking anything, but I just wanted to make sure we had an issue filed.

Right now the 404 response is HTML. How about we make it JSON?

It doesn't seem like anybody really depends on this right now but it is technically a required part of the spec.

Just before release. At the least, we probably now need an index on (createdAt, formId) on submissions.

One must look in the body for repeat control tags and do the association that way.

This may be on @clint-tseng's radar already, but right now, I think Base.prototype.update() always resolves the promise with false. db.update() seems to resolve the promise with the number of affected rows, not an object with a rowCount property. For example:

> const { connect } = require('./lib/model/database');
> const db = connect();
> db.
    update({ xmlFormId: 'newFormId' }).
    into('forms').
    where({ id: 1 }).
    then(result => console.log(result));
1 // Not { rowCount: 1 }

Since Base.prototype.update() looks for a rowCount property but will never find one, I think the promise will always resolve with false.

Right now we do weird dirty tricks like mapping the objects through functions that assert a property then eliminate that property, so that in the end we can do an eql({ … }) check against the stable data that remains.

For one, these functions get awkward applied to non-lists.
For two, the implicit data munging (even though it is immutable transformation) is troublesome.
For three, it is highly repetitive.

@clint-tseng and I are thinking of starting out with Vue.js. It supports "all browsers that are ES5-compliant (IE8 and below are not supported)" (see here). I think that should be fine, especially since Jubilant will be the server component, but we wanted to double-check that that'd work alright…

When Jubilant outputs multiple .csv files, how closely should their format match Briefcase? I tend to think that they should match as closely as possible so that extensions and workflows based on the Briefcase output will continue to work with Jubilant. How much work would that entail?

The OData one is probably better?

I didn't see an existing issue/PR for this, so I just wanted to add a +1 to @lognaturel's comment in #32:

The commit comments are really helpful and I would like to see a little bit of that become comments and/or dev docs. I know that @cxlt you were reluctant to get started with dev docs because understandably you expect things will change but I think just a little bit of high-level narrative now would help. A concrete way to start would be to take Instance.with(ActeeTrait)(({ Grant, actors, all, grants, simply }) => class and upack it a little bit. Link out to a good narrative on traits, describe that package.js is the dependency injection container, a paragraph on Option, things like that. I think a few minutes on that would help a lot with giving someone an idea of where to start. It doesn't have to go in this PR.

It's just sort of sprawly right now.

I'm noticing a couple of null fields in responses from /forms when I request extended metadata:

• formId. I see that forApi() in instance/form.js excludes the id field. I think the null value is coming from submission_stats.formId.
• submissions. This seems to be null rather than zero when there are no submissions. I could see a case being made for either one, but I'd lean toward returning zero.

This isn't blocking anything on the Adventure side, but I thought I'd mention it as a possible future to-do.

We are soft-deleting accounts and formdata, so that recovery is possible. But this means that if an account is deleted, a new one cannot be created at the same address, as the unique constraint still applies.

The likely solution is to bind the unique constraint on a compound (email, deletedAt). But because account deletion isn't actually in the initial release, we can punt.

Many TODOs in the resources/config.js code.

Right now we use -1 in some places.

Because autoincrement sequences do not rollback with a transaction, and therefore generated ids are dependent on test order and inclusion. So the best we can do is assert a number.

Right now, there are many tests that assert a specific number, which is dependent on the other tests involving that model running in a particular order and inclusion. The whole suite works stably, so this isn't catastrophically urgent, but should be addressed, probably at the same time as #55.

#10 and #26 add unit/integration tests for the database as a whole. This issue is to add tests for the Form and Submission models specifically.