Validate contribution docs against schema #92

2019-04-10T16:38:30Z

raucao commented

2019-04-10 16:38:30 +00:00

(Migrated from github.com)

Currently requires an open PR branch for the schemas, which is adding date and time for contributions.

refs #30

Currently requires [an open PR branch](https://github.com/67P/kosmos-schemas/pull/5) for the schemas, which is adding date and time for contributions. refs #30

bumi (Migrated from github.com) reviewed 2019-04-10 16:49:42 +00:00

bumi (Migrated from github.com) left a comment

yay, and wow such regexp! :D

lib/contracts/contribution.js

						
				@@ -3,0 +13,4 @@

				    const timeRegexp = /^([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9]|60)(\.[0-9]+)?(([Zz])|([\+|\-]([01][0-9]|2[0-3]):[0-5][0-9]))$/;

				    return timeRegexp.test(value) ? null : "A valid ISO 8601 full-time string is expected";

				  }

				})

bumi (Migrated from github.com) commented

2019-04-10 16:47:34 +00:00

afaik we use the same format for the proposals and a proposal once accepted creates a contribution.
So we should probably add this also in the proposal wrapper.

afaik we use the same format for the proposals and a proposal once accepted creates a contribution. So we should probably add this also in the proposal wrapper.

scripts/add-contribution.js

bumi (Migrated from github.com) commented

2019-04-10 16:49:02 +00:00

does this work? thought the callback should get an error object.

raucao (Migrated from github.com) reviewed 2019-04-10 16:52:28 +00:00

lib/contracts/contribution.js

						
				@@ -3,0 +13,4 @@

				    const timeRegexp = /^([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9]|60)(\.[0-9]+)?(([Zz])|([\+|\-]([01][0-9]|2[0-3]):[0-5][0-9]))$/;

				    return timeRegexp.test(value) ? null : "A valid ISO 8601 full-time string is expected";

				  }

				})

raucao (Migrated from github.com) commented

2019-04-10 16:52:28 +00:00

Yeah, but proposals aren't really part of MVP.

raucao (Migrated from github.com) reviewed 2019-04-10 16:53:05 +00:00

scripts/add-contribution.js

raucao (Migrated from github.com) commented

2019-04-10 16:53:05 +00:00

We need to reject with the tv4 error object either way. It does show the error and doesn't stall. So it works well enough to be usable, yes.

bumi (Migrated from github.com) approved these changes 2019-04-10 21:29:58 +00:00

lib/contracts/contribution.js

						
				@@ -3,0 +13,4 @@

				    const timeRegexp = /^([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9]|60)(\.[0-9]+)?(([Zz])|([\+|\-]([01][0-9]|2[0-3]):[0-5][0-9]))$/;

				    return timeRegexp.test(value) ? null : "A valid ISO 8601 full-time string is expected";

				  }

				})

bumi (Migrated from github.com) commented

2019-04-10 21:28:42 +00:00

yeah, though it might be worth to move this into a separate file that exports validator
and then do a const validator = require('validator') ?

yeah, though it might be worth to move this into a separate file that exports validator and then do a `const validator = require('validator')` ?

lib/contracts/contribution.js

bumi (Migrated from github.com) commented

2019-04-10 21:29:07 +00:00

I somewhat dislike that we do a JSON.stringify in the ContributionSerializer and then do a JSON.parse again to validate it.
maybe we can have two function in the ContributionSerializer?
of even do the validation in the ContributionSerializer?

something like:

const c = Contribtion.new(contribtionAttr);
c.isValid() // calls the validator.validate() function
c.error // returns validator.error
c.serialize() // same as now

but we can refactor that later I guess.

I somewhat dislike that we do a `JSON.stringify` in the ContributionSerializer and then do a `JSON.parse` again to validate it. maybe we can have two function in the ContributionSerializer? of even do the validation in the ContributionSerializer? something like: ```js const c = Contribtion.new(contribtionAttr); c.isValid() // calls the validator.validate() function c.error // returns validator.error c.serialize() // same as now ``` but we can refactor that later I guess.

raucao (Migrated from github.com) reviewed 2019-04-11 06:39:34 +00:00

lib/contracts/contribution.js

raucao (Migrated from github.com) commented

2019-04-11 06:39:33 +00:00

Yes, makes sense! We can refactor it when adding validation for contributor details.

(I also thought about adding it to the schemas library, but decided against it, because I think it shouldn't prescribe what to do with the schemas and then load that additional code.)

Yes, makes sense! We can refactor it when adding validation for contributor details. (I also thought about adding it to the schemas library, but decided against it, because I think it shouldn't prescribe what to do with the schemas and then load that additional code.)

fsmanuel (Migrated from github.com) reviewed 2019-04-11 07:45:03 +00:00

lib/contracts/contribution.js

fsmanuel (Migrated from github.com) commented

2019-04-11 07:45:03 +00:00

Maybe something like:

// This tv4 api feels strange...
const [valid, validator] = ContributionSerializer.validate(contribtionAttr);
if (!valid) { return Promise.reject(validator.error); }

.validate should use _serialize() to do the work and serialize() can use the private method as well but returns the JSON string...

I think the overhead of _serialize() is not much so we can do it twice.

Maybe something like: ```js // This tv4 api feels strange... const [valid, validator] = ContributionSerializer.validate(contribtionAttr); if (!valid) { return Promise.reject(validator.error); } ``` `.validate` should use `_serialize()` to do the work and `serialize()` can use the private method as well but returns the JSON string... I think the overhead of `_serialize()` is not much so we can do it twice.

fsmanuel (Migrated from github.com) reviewed 2019-04-11 07:47:26 +00:00

lib/contracts/contribution.js

						
				@@ -3,0 +13,4 @@

				    const timeRegexp = /^([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9]|60)(\.[0-9]+)?(([Zz])|([\+|\-]([01][0-9]|2[0-3]):[0-5][0-9]))$/;

				    return timeRegexp.test(value) ? null : "A valid ISO 8601 full-time string is expected";

				  }

				})