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

Rename setPaymentDisabled to setPaymentStatus #474

Merged
merged 5 commits into from Oct 16, 2018

Conversation

Projects
None yet
4 participants
@IvanTheGreatDev
Copy link
Contributor

IvanTheGreatDev commented Oct 9, 2018

Also changes constant to view app-wide and renamed app-wide from disabled to inactive for the payment status.

#374

Renames setPaymentDisabled to setPaymentStatus
Also changes constant to view app-wide
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 9, 2018

Coverage Status

Coverage remained the same at 96.212% when pulling 271b89f on IvanTheGreatDev:hotfix/rename-setpaymentdisabled into 67161d1 on aragon:master.

@IvanTheGreatDev IvanTheGreatDev changed the title Renames setPaymentDisabled to setPaymentStatus Rename setPaymentDisabled to setPaymentStatus Oct 10, 2018

@sohkai
Copy link
Member

sohkai left a comment

Nice work! I left a few comments, but this is looking good.

Do you think active / inactive is better terminology than enabled / disabled?

Show resolved Hide resolved apps/finance/artifact.json
*/
function setPaymentDisabled(uint256 _paymentId, bool _disabled)
function setPaymentStatus(uint256 _paymentId, bool _active)
external
authP(DISABLE_PAYMENTS_ROLE, arr(_paymentId))

This comment has been minimized.

@sohkai

sohkai Oct 11, 2018

Member

We should also rename this role, so it's less confusing. It can actually enable or disable existing payments, so I'd rename it to MANAGE_PAYMENTS_ROLE.

Although @izqui we should probably think of a way to completely disable a payment at some point (or remove the ability to re-enable payments).

@@ -192,7 +192,7 @@ contract MiniMeToken is Controlled {
require((_to != 0) && (_to != address(this)));
// If the amount being transfered is more than the balance of the
// account the transfer returns false
var previousBalanceFrom = balanceOfAt(_from, block.number);
uint256 previousBalanceFrom = balanceOfAt(_from, block.number);

This comment has been minimized.

@sohkai

sohkai Oct 11, 2018

Member

Could you revert the changes to the MiniMeToken here?

I'm not opposed to fixing the compiler warnings here, but we'd also like it to stay as closely as possible to the original version and we should have a separate discussion if we should just update to MMT_0.2.

This comment has been minimized.

@IvanTheGreatDev

IvanTheGreatDev Oct 11, 2018

Contributor

Sure thing!

Show resolved Hide resolved apps/finance/test/finance.js

IvanTheGreatDev added some commits Oct 11, 2018

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Oct 11, 2018

Just to re-surface this from #474 (comment):

Although @izqui we should probably think of a way to completely disable a payment at some point (or remove the ability to re-enable payments).

@IvanTheGreatDev

This comment has been minimized.

Copy link
Contributor

IvanTheGreatDev commented Oct 12, 2018

Should be good to go now! 😄

@IvanTheGreatDev

This comment has been minimized.

Copy link
Contributor

IvanTheGreatDev commented Oct 16, 2018

What is this PR waiting on?

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Oct 16, 2018

Just a final review, @IvanTheGreatDev! Just added 51539ca passing whether we are setting the payment as active as a parameter to the role.

Thanks for the contribution :)

@izqui

izqui approved these changes Oct 16, 2018

@izqui izqui merged commit ebc5b04 into aragon:master Oct 16, 2018

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@IvanTheGreatDev IvanTheGreatDev deleted the IvanTheGreatDev:hotfix/rename-setpaymentdisabled branch Oct 16, 2018

@IvanTheGreatDev IvanTheGreatDev restored the IvanTheGreatDev:hotfix/rename-setpaymentdisabled branch Oct 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment