kredits/kredits-web
9
0
Fork 0
Code Issues 22 Pull Requests 1 Actions Releases 18 Activity

Move all metadata to IPFS with proper schemas #16

Merged
raucao merged 23 commits from feature/ipfs-metadata into master 2017-06-08 22:28:15 +00:00
Conversation 22 Commits 23 Files Changed 24
+478 -177
raucao commented 2017-06-05 20:04:40 +00:00 (Migrated from github.com)
Copy Link
Copy Source

This refactors the app to use the new schemas I created for storing/retrieving all metadata in/from IPFS.

  • Store contributor data
  • Retrieve (all) contributor data
  • Store proposal data
  • Retrieve proposal data
This refactors the app to use the [new schemas](https://github.com/67P/kosmos-schemas/) I created for storing/retrieving all metadata in/from IPFS. * [x] Store contributor data * [x] Retrieve (all) contributor data * [x] Store proposal data * [x] Retrieve proposal data
raucao commented 2017-06-07 10:47:51 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Added color-coded hearts for the contribution type:

screenshot from 2017-06-07 12-44-28

Added color-coded hearts for the contribution type: ![screenshot from 2017-06-07 12-44-28](https://user-images.githubusercontent.com/842/26874859-835e1230-4b7f-11e7-8680-0b62f076a507.png)
raucao commented 2017-06-07 10:55:46 +00:00 (Migrated from github.com)
Copy Link
Copy Source

We're not showing more than the color coding for the kind, as of now, but there's no need to keep this in a branch anymore. It's actually important we get the IPFS doc creation for both contributors and proposals into master asap, so please review!

We're not showing more than the color coding for the kind, as of now, but there's no need to keep this in a branch anymore. It's actually important we get the IPFS doc creation for both contributors and proposals into master asap, so please review!
raucao commented 2017-06-07 11:02:27 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I hacked in the contribution details as title to be shown on hovering the proposal items. Works like a charm and so useful. :)

I hacked in the contribution details as title to be shown on hovering the proposal items. Works like a charm and so useful. :)
galfert (Migrated from github.com) requested changes 2017-06-08 14:11:03 +00:00
galfert (Migrated from github.com) left a comment
Copy Link
Copy Source

Added some notes and questions. Other than that, it looks good.

Added some notes and questions. Other than that, it looks good.
app/components/add-contributor/component.js
@@ -1,41 +1,58 @@
import Ember from 'ember';
galfert (Migrated from github.com) commented 2017-06-08 13:40:36 +00:00
Copy Link
Copy Source

I think this is supposed to be newContributor.url instead of newContributor.name.

I think this is supposed to be `newContributor.url` instead of `newContributor.name`.
galfert (Migrated from github.com) commented 2017-06-08 13:43:45 +00:00
Copy Link
Copy Source

Not sure about this, but maybe it makes sense to add kredits.web3Instance as dependent key as well? So the property gets recalculated when kredits.web3Instance changes. Thinking of that race condition where the user provided web3 is only available after a while.

Not sure about this, but maybe it makes sense to add `kredits.web3Instance` as dependent key as well? So the property gets recalculated when `kredits.web3Instance` changes. Thinking of that race condition where the user provided web3 is only available after a while.
app/components/add-proposal/component.js
@@ -13,47 +14,52 @@ export default Component.extend({
kredits: service(),
galfert (Migrated from github.com) commented 2017-06-08 13:52:17 +00:00
Copy Link
Copy Source

You should always use a radix parameter of 10 with parseInt.

You should always use a radix parameter of `10` with `parseInt`.
app/components/add-proposal/component.js
@@ -28,3 +29,2 @@
// TODO
return true;
return isPresent(this.get('proposal.url'));
}),
galfert (Migrated from github.com) commented 2017-06-08 13:50:55 +00:00
Copy Link
Copy Source

Is this supposed to be kredits.web3 now? Because in the add-contributor component it's still using kredits.web3Instance.

Is this supposed to be `kredits.web3` now? Because in the `add-contributor` component it's still using `kredits.web3Instance`.
app/components/add-proposal/component.js
galfert (Migrated from github.com) commented 2017-06-08 13:54:36 +00:00
Copy Link
Copy Source

I think this is missing a return; after the alert now, so it doesn't continue trying to save the data when it's invalid.

I think this is missing a `return;` after the `alert` now, so it doesn't continue trying to save the data when it's invalid.
app/services/ipfs.js
@@ -26,6 +26,9 @@ export default Ember.Service.extend({
getFile(hash) {
galfert (Migrated from github.com) commented 2017-06-08 14:06:28 +00:00
Copy Link
Copy Source

I think you want to throw the error here. Otherwise the getFile Promise will resolve with the err as value instead of being rejected.

I think you want to `throw` the error here. Otherwise the `getFile` Promise will resolve with the `err` as value instead of being rejected.
raucao (Migrated from github.com) reviewed 2017-06-08 14:23:11 +00:00
app/components/add-proposal/component.js
@@ -28,3 +29,2 @@
// TODO
return true;
return isPresent(this.get('proposal.url'));
}),
raucao (Migrated from github.com) commented 2017-06-08 14:23:11 +00:00
Copy Link
Copy Source

Not "now", but in general, yes. I don't know why it's not in the add-contributor component.

Not "now", but in general, yes. I don't know why it's not in the add-contributor component.
raucao (Migrated from github.com) reviewed 2017-06-08 14:23:30 +00:00
app/components/add-proposal/component.js
@@ -13,47 +14,52 @@ export default Component.extend({
kredits: service(),
raucao (Migrated from github.com) commented 2017-06-08 14:23:30 +00:00
Copy Link
Copy Source

What does that mean?

What does that mean?
raucao (Migrated from github.com) reviewed 2017-06-08 14:24:58 +00:00
app/components/add-proposal/component.js
raucao (Migrated from github.com) commented 2017-06-08 14:24:58 +00:00
Copy Link
Copy Source

Yes, totally.

Yes, totally.
galfert (Migrated from github.com) reviewed 2017-06-08 14:36:58 +00:00
app/components/add-proposal/component.js
@@ -13,47 +14,52 @@ export default Component.extend({
kredits: service(),
galfert (Migrated from github.com) commented 2017-06-08 14:36:58 +00:00
Copy Link
Copy Source

The second parameter of parseInt should always be 10 if you want it to be parsed as decimal number. So in this case it should be parseInt(this.get('proposal.amount'), 10).
Otherwise, in certain situations the number will be interpreted with a different base (e.g. when the number starts with a 0, it will be octal).

The second parameter of `parseInt` should always be `10` if you want it to be parsed as decimal number. So in this case it should be `parseInt(this.get('proposal.amount'), 10)`. Otherwise, in certain situations the number will be interpreted with a different base (e.g. when the number starts with a 0, it will be octal).
raucao (Migrated from github.com) reviewed 2017-06-08 14:40:03 +00:00
app/services/ipfs.js
@@ -26,6 +26,9 @@ export default Ember.Service.extend({
getFile(hash) {
raucao (Migrated from github.com) commented 2017-06-08 14:40:03 +00:00
Copy Link
Copy Source

Good catch! Pun indubitably intended.

Good catch! Pun indubitably intended.
raucao commented 2017-06-08 14:43:13 +00:00 (Migrated from github.com)
Copy Link
Copy Source

@galfert Thanks. I addressed all comments and asked about the one I don't understand.

@galfert Thanks. I addressed all comments and asked about the one I don't understand.
galfert commented 2017-06-08 14:52:18 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I addressed all comments and asked about the one I don't understand.

You mean the one about parseInt? I had already answered that one.

> I addressed all comments and asked about the one I don't understand. You mean the one about `parseInt`? I had already answered that one.
raucao (Migrated from github.com) reviewed 2017-06-08 14:54:04 +00:00
app/components/add-proposal/component.js
@@ -13,47 +14,52 @@ export default Component.extend({
kredits: service(),
raucao (Migrated from github.com) commented 2017-06-08 14:54:04 +00:00
Copy Link
Copy Source

I see, thanks.

I see, thanks.
raucao commented 2017-06-08 14:54:43 +00:00 (Migrated from github.com)
Copy Link
Copy Source

You mean the one about parseInt? I had already answered that one.

That was too quick to even notice or get a notification. :)

Added the param.

> You mean the one about parseInt? I had already answered that one. That was too quick to even notice or get a notification. :) Added the param.
galfert (Migrated from github.com) approved these changes 2017-06-08 15:02:20 +00:00
galfert (Migrated from github.com) reviewed 2017-06-08 15:03:56 +00:00
app/components/add-contributor/component.js
@@ -1,41 +1,58 @@
import Ember from 'ember';
galfert (Migrated from github.com) commented 2017-06-08 15:03:56 +00:00
Copy Link
Copy Source

I think you missed this one.

I think you missed this one.
galfert commented 2017-06-08 15:05:50 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I think you missed one comment (see https://github.com/67P/kredits-web/pull/16#discussion_r120913421). The rest looks all good now.

I think you missed one comment (see https://github.com/67P/kredits-web/pull/16#discussion_r120913421). The rest looks all good now.
raucao commented 2017-06-08 15:06:41 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Done.

Done.
bumi (Migrated from github.com) reviewed 2017-06-08 21:02:31 +00:00
app/components/add-contributor/component.js
@@ -28,0 +44,4 @@
isValid: computed.and(
'isValidAddress',
'isValidName',
bumi (Migrated from github.com) commented 2017-06-08 21:02:31 +00:00
Copy Link
Copy Source

could we validate these using some json schema validators?

could we validate these using some json schema validators?
raucao (Migrated from github.com) reviewed 2017-06-08 21:07:23 +00:00
app/components/add-contributor/component.js
@@ -28,0 +44,4 @@
isValid: computed.and(
'isValidAddress',
'isValidName',
raucao (Migrated from github.com) commented 2017-06-08 21:07:23 +00:00
Copy Link
Copy Source

In theory yes (there's e.g. "format": "uri"), but it'd be much easier here to just have some simple regexp for HTTP URIs imo (which also keeps the build size down).

In theory yes (there's e.g. `"format": "uri"`), but it'd be much easier here to just have some simple regexp for HTTP URIs imo (which also keeps the build size down).
bumi (Migrated from github.com) reviewed 2017-06-08 21:16:32 +00:00
app/services/kredits.js
bumi (Migrated from github.com) commented 2017-06-08 21:16:32 +00:00
Copy Link
Copy Source

as a note: I thought about somehow generating an ID from within the contract.

as a note: I thought about somehow generating an ID from within the contract.
raucao (Migrated from github.com) reviewed 2017-06-08 21:22:32 +00:00
app/services/kredits.js
raucao (Migrated from github.com) commented 2017-06-08 21:22:32 +00:00
Copy Link
Copy Source

I'd prefer that. Until we have it, this is the next-best thing. :)

I'd prefer that. Until we have it, this is the next-best thing. :)
raucao commented 2017-06-08 22:28:10 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Thanks everyone!

Thanks everyone!
Sign in to join this conversation.
Reviewers
No Reviewers
galfert
bumi
raucao
Labels
Clear labels
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
Assignees
Clear assignees
bkero (bkero) bumi (bumi) fsmanuel (fsmanuel) galfert (galfert) greg (Greg) haythem (haythem) hueso (hueso) raucao (Râu Cao) slvrbckt (Nick Jennings)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/kredits-web#16
Delete Branch "feature/ipfs-metadata"

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?