kredits/contracts
kredits/contracts
9
1
Fork 0
Code Issues 21 Pull Requests Actions Projects 1 Releases 8 Activity

Validate contribution docs against schema #92

Merged
raucao merged 1 commits from feature/contribution_schema into master 2019-04-11 06:40:39 +00:00
Conversation 9 Commits 1 Files Changed 5 +116 -90
raucao commented 2019-04-10 16:38:30 +00:00 (Migrated from github.com)
Copy Link

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
Copy Link

yay, and wow such regexp! :D

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
Copy Link

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
Copy Link

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

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
Copy Link

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

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
Copy Link

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.

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
Copy Link

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
Copy Link

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
Copy Link

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
Copy Link

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";
}
})
fsmanuel (Migrated from github.com) commented 2019-04-11 07:47:26 +00:00
Copy Link

I think it makes a lot of sense to have everything in the serializers. Still not sure how best to implement the validation in the serializer...

I think it makes a lot of sense to have everything in the serializers. Still not sure how best to implement the validation in the serializer...
raucao (Migrated from github.com) reviewed 2019-04-11 11:09:27 +00:00
lib/contracts/contribution.js
raucao (Migrated from github.com) commented 2019-04-11 11:09:26 +00:00
Copy Link

Already came up with an even cleaner solution. Promisify ALL THE THINGS. ;)

Already came up with an even cleaner solution. Promisify ALL THE THINGS. ;)
raucao (Migrated from github.com) reviewed 2019-04-11 11:10:26 +00:00
lib/contracts/contribution.js
raucao (Migrated from github.com) commented 2019-04-11 11:10:26 +00:00
Copy Link

d953141f52/lib/contracts/contribution.js (L51-L54)

https://github.com/67P/kredits-contracts/blob/d953141f52d515dbcba4216c5eab9111c58467a2/lib/contracts/contribution.js#L51-L54
Sign in to join this conversation.
Reviewers
No Reviewers
bumi
Labels
Clear labels
good first issue
Good for newcomers
ipfs
rsk
scaling
bug
Something is not working
dev environment
Config, builds, CI, deployment, etc.
docs
Documentation
duplicate
This issue or pull request already exists
enhancement
Improving existing functionality
feature
New functionality
idea
Something to consider
invalid
Not a bug
kredits-1
Small contribution
kredits-2
Medium contribution
kredits-3
Large contribution
question
Looking for an answer
release
major
release
minor
release
patch
security
All your base are belong to us
ui/ux
User interface, process design, etc.
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
bkero bumi fsmanuel galfert greg haythem hueso raucao slvrbckt
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/contracts#92
No description provided.
Delete Branch "feature/contribution_schema"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?