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

Refactor proposals #38

Merged
fsmanuel merged 6 commits from refactor/proposal into master 2018-04-08 12:02:28 +00:00
Conversation 13 Commits 6 Files Changed 11
+295 -307
fsmanuel commented 2018-04-08 00:03:40 +00:00 (Migrated from github.com)
Copy Link
Copy Source

@skddc can you help me to understand how the queryParams are used? I removed them for now (see the commit message) but I'm happy to readd them.

@skddc can you help me to understand how the queryParams are used? I removed them for now (see the commit message) but I'm happy to readd them.
fsmanuel (Migrated from github.com) reviewed 2018-04-08 00:05:01 +00:00
app/controllers/proposals/new.js
fsmanuel (Migrated from github.com) commented 2018-04-08 00:05:00 +00:00
Copy Link
Copy Source

@bumi we need to look into hubot and how we should do the contributor look up there. I think if we have some kind of caching it would be better to have it in the kredits module

@bumi we need to look into hubot and how we should do the contributor look up there. I think if we have some kind of caching it would be better to have it in the kredits module
bumi (Migrated from github.com) reviewed 2018-04-08 00:15:25 +00:00
app/controllers/proposals/new.js
bumi (Migrated from github.com) commented 2018-04-08 00:15:25 +00:00
Copy Link
Copy Source

I'd remove the contributor ipfsHash from the proposal and only have the kredits id stored there. With that one the latest profile ipfs hash can be retrieved from the contract. This would also make sure that one always gets the latest profile hash of the contributor.

hope I did not forget some reason why the hash is stored there.

I'd remove the contributor ipfsHash from the proposal and only have the kredits id stored there. With that one the latest profile ipfs hash can be retrieved from the contract. This would also make sure that one always gets the latest profile hash of the contributor. hope I did not forget some reason why the hash is stored there.
raucao (Migrated from github.com) reviewed 2018-04-08 10:37:27 +00:00
app/controllers/proposals/new.js
raucao (Migrated from github.com) commented 2018-04-08 10:37:27 +00:00
Copy Link
Copy Source

It is stored there, because you need to be able to access the profile at the time of the proposal. Otherwise someone can do nasty things or just plain accidental bad things.

It is stored there, because you need to be able to access the profile at the time of the proposal. Otherwise someone can do nasty things or just plain accidental bad things.
raucao commented 2018-04-08 10:49:14 +00:00 (Migrated from github.com)
Copy Link
Copy Source

@skddc can you help me to understand how the queryParams are used? I removed them for now (see the commit message) but I'm happy to readd them.

They are used for linking a new proposal creation form, with any of the fields pre-set by the URL.

> @skddc can you help me to understand how the queryParams are used? I removed them for now (see the commit message) but I'm happy to readd them. They are used for linking a new proposal creation form, with any of the fields pre-set by the URL.
raucao (Migrated from github.com) reviewed 2018-04-08 11:02:49 +00:00
raucao (Migrated from github.com) left a comment
Copy Link
Copy Source

Nice!

I can't really test this in my browser at the moment, because it's using new Ethereum contracts, right? Just added a few comments for some minor things.

Nice! I can't really test this in my browser at the moment, because it's using new Ethereum contracts, right? Just added a few comments for some minor things.
app/services/kredits.js
raucao (Migrated from github.com) commented 2018-04-08 10:53:41 +00:00
Copy Link
Copy Source

This looks a bit strange to me. Is there a benefit to not just importing the model and doing a create() or something? (I'm not actually sure what's really happening here though.)

This looks a bit strange to me. Is there a benefit to not just importing the model and doing a `create()` or something? (I'm not actually sure what's really happening here though.)
app/services/kredits.js
raucao (Migrated from github.com) commented 2018-04-08 10:56:18 +00:00
Copy Link
Copy Source

Just a formatting thing: if it's only one function argument, we don't need the parentheses around it. So it looks a bit cleaner and reads just a tiny little bit easier.

Just a formatting thing: if it's only one function argument, we don't need the parentheses around it. So it looks a bit cleaner and reads just a tiny little bit easier.
app/services/kredits.js
raucao (Migrated from github.com) commented 2018-04-08 10:57:28 +00:00
Copy Link
Copy Source

I think the hash splitting and combining should not be done in here, but needs to in our kredits npm module, so we can use it from other programs as well.

I think the hash splitting and combining should not be done in here, but needs to in our kredits npm module, so we can use it from other programs as well.
tests/unit/models/proposal-test.js
raucao (Migrated from github.com) commented 2018-04-08 11:00:54 +00:00
Copy Link
Copy Source

The validations seem to be missing in the new serializer tests.

The validations seem to be missing in the new serializer tests.
fsmanuel (Migrated from github.com) reviewed 2018-04-08 11:33:32 +00:00
app/services/kredits.js
fsmanuel (Migrated from github.com) commented 2018-04-08 11:33:32 +00:00
Copy Link
Copy Source

The problem with the .create() is that we can not inject services because the model doesn't have a container. I'm unhappy with this as well and will try to figure out a better way.

The problem with the `.create()` is that we can not inject services because the model doesn't have a container. I'm unhappy with this as well and will try to figure out a better way.
fsmanuel (Migrated from github.com) reviewed 2018-04-08 11:35:09 +00:00
app/services/kredits.js
fsmanuel (Migrated from github.com) commented 2018-04-08 11:35:09 +00:00
Copy Link
Copy Source

What we try right now is that we cleanup the kredits service and and extract all the common logic from the service in the npm module. That will be our next step.

What we try right now is that we cleanup the kredits service and and extract all the common logic from the service in the npm module. That will be our next step.
bumi (Migrated from github.com) reviewed 2018-04-08 11:36:19 +00:00
app/controllers/proposals/new.js
bumi (Migrated from github.com) commented 2018-04-08 11:36:19 +00:00
Copy Link
Copy Source

hmmm. but all data is stored in the contract and that one holds the "truth".
the proposal IPFS hash is retrieved from the contract and through the contracts the contributor ipfs hash can be retrieved. (the latest one, but also thanks to blockchain the one at a specific time in the past - when the proposal was created).

To load data might be one more step: get the contributor by id from the contract and then access the contributor profile from IPFS.

I think at least we should also add the kredits contributor id to the proposal ipfs data to be able to get to the contributor in the contract from the ipfs proposal data.

hmmm. but all data is stored in the contract and that one holds the "truth". the proposal IPFS hash is retrieved from the contract and through the contracts the contributor ipfs hash can be retrieved. (the latest one, but also thanks to blockchain the one at a specific time in the past - when the proposal was created). To load data might be one more step: get the contributor by id from the contract and then access the contributor profile from IPFS. I think at least we should also add the kredits contributor id to the proposal ipfs data to be able to get to the contributor in the contract from the ipfs proposal data.
fsmanuel (Migrated from github.com) reviewed 2018-04-08 11:36:41 +00:00
tests/unit/models/proposal-test.js
fsmanuel (Migrated from github.com) commented 2018-04-08 11:36:41 +00:00
Copy Link
Copy Source

Yes. I'll add it in addContributor and addProposal and use v4 to do it. I think it's cleaner to validate before we save instead of while we serialize.

Yes. I'll add it in `addContributor` and `addProposal` and use `v4` to do it. I think it's cleaner to validate before we save instead of while we serialize.
raucao (Migrated from github.com) reviewed 2018-04-08 11:43:26 +00:00
app/services/kredits.js
raucao (Migrated from github.com) commented 2018-04-08 11:43:26 +00:00
Copy Link
Copy Source

Cool.

Cool.
raucao (Migrated from github.com) reviewed 2018-04-08 11:43:50 +00:00
tests/unit/models/proposal-test.js
raucao (Migrated from github.com) commented 2018-04-08 11:43:50 +00:00
Copy Link
Copy Source

Sounds good.

Sounds good.
bumi commented 2018-04-08 11:45:25 +00:00 (Migrated from github.com)
Copy Link
Copy Source

yep, it uses the new contracts and the new truffle setup. That makes it way easier to develop and manage the contracts. Right now those are in: https://github.com/67P/truffle-kredits - but should replace the kredits-contract package.
Truffle has made great improvements in the last months and the ganache-cli is a great local network simulator and can easily be installed with npm.

There is setup information here:
https://github.com/67P/kredits-web#working-with-locally-deployed-contracts

Basically it is:

  • run and seed ganache (the local network simulator) (in truffle-kredits)
  • run an offline ipfs daemon
  • link the package
  • run kredits-web with ember
yep, it uses the new contracts and the new truffle setup. That makes it way easier to develop and manage the contracts. Right now those are in: https://github.com/67P/truffle-kredits - but should replace the kredits-contract package. Truffle has made great improvements in the last months and the ganache-cli is a great local network simulator and can easily be installed with npm. There is setup information here: https://github.com/67P/kredits-web#working-with-locally-deployed-contracts Basically it is: * run and seed ganache (the local network simulator) (in truffle-kredits) * run an offline ipfs daemon * link the package * run kredits-web with ember
bumi (Migrated from github.com) approved these changes 2018-04-08 12:01:42 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
fsmanuel
raucao
bumi
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#38
Delete Branch "refactor/proposal"

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?