Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor contract to let editors just publish stuff #90

Merged
merged 6 commits into from
Apr 23, 2018

Conversation

walfly
Copy link
Contributor

@walfly walfly commented Apr 13, 2018

No description provided.

@walfly walfly force-pushed the editor-can-publish branch 2 times, most recently from 5c6a6cc to 6ebdafc Compare April 13, 2018 19:03
mapping(uint => Content) private content;
mapping(uint => bool) private waiting;
mapping(uint => bool) private approved;
mapping(uint => Revision) private content;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can make this mapping public, actually, and get rid of the getters (see PR 86 changes to Listing/Challenge/Appeal mappings)

event ContentProposed(address indexed author, uint indexed id);
event ContentApproved(uint id);
event ContentDenied(uint id);
event ContentAdded(address indexed editor, uint indexed id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added -> Published might be more clear? Also might be worth it to add a bit more info to this event (url would be helpful)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RevisionPublished? Then we can have a separate event that fires in addition to this one if the article is brand new, as opposed to just a revision

@@ -11,6 +11,7 @@ const expect = chai.expect;

const FIRST_NEWSROOM_NAME = "TEST NAME, PLEASE IGNORE";
const SOME_URI = "http://thiistest.uri";
const SOME_HASH = web3.sha3();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this you should be able to just do const SOME_HASH = "0x01"

@@ -56,41 +32,28 @@ contract Newsroom is ACL {
_removeRole(who, role);
}

function proposeContent(string contentUri) public requireRole(ROLE_REPORTER) returns (uint) {
function addContent(string contentUri, bytes32 contentHash) public requireRole(ROLE_EDITOR) returns (uint) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

publishContent() feels better

@walfly walfly added the dont merge pr not ready for merge label Apr 13, 2018
@walfly
Copy link
Contributor Author

walfly commented Apr 13, 2018

adding changes to core

@walfly walfly removed the dont merge pr not ready for merge label Apr 19, 2018
@walfly walfly force-pushed the editor-can-publish branch 7 times, most recently from ee685bc to ca4a90a Compare April 23, 2018 16:39
@@ -14,7 +14,7 @@ const debug = Debug("civil:main");

export interface CivilOptions {
web3Provider?: Web3.Provider;
contentProvider?: ContentProvider;
ContentProvider?: ContentProviderCreator;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is ContentProvider capitalized here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a Class definition not an instance

ipfs.add(Buffer.from(content), (err: any, ipfsHash: any) => {
resolve(this.scheme() + "://" + ipfsHash[0].path);
resolve({ uri: this.scheme() + "://" + ipfsHash[0].path, hash: ipfsHash });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ipfsHash here is actually poorly named. ipfsHash[0].path gets the actual hash, I believe

@walfly
Copy link
Contributor Author

walfly commented Apr 23, 2018

Wooooo

@walfly walfly merged commit 8ac5172 into master Apr 23, 2018
@ritave ritave deleted the editor-can-publish branch April 23, 2018 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants