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

(Fix) Add address to whitelist in manage screen #672

Merged
merged 19 commits into from
Mar 14, 2018

Conversation

fernandomg
Copy link
Contributor

Closes #596

In this PR whitelistElements collection was removed and only uses whitelist.

Style was applied for the whitelist elements in the manage screen, so the user can recognize what addresses are being modified, added or untouched.

image

Things to fix that I found:

  1. The way how duplicated addresses are being checked. As the same address can have upper and lower cases, this may lead errors.
  2. min and max values are not being checked, so a user can enter a min value greater than max. The tx will fail, but it won't be clear why.

@ghost ghost assigned fernandomg Mar 8, 2018
@ghost ghost added the in progress label Mar 8, 2018
@coveralls
Copy link

coveralls commented Mar 8, 2018

Pull Request Test Coverage Report for Build 1644

  • 27 of 48 (56.25%) changed or added relevant lines in 6 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+1.1%) to 15.468%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/crowdsale/index.js 0 1 0.0%
src/components/manage/utils.js 0 3 0.0%
src/components/Common/WhitelistItem.js 0 4 0.0%
src/components/manage/index.js 0 13 0.0%
Files with Coverage Reduction New Missed Lines %
src/components/manage/utils.js 1 0.0%
Totals Coverage Status
Change from base Build 1640: 1.1%
Covered Lines: 375
Relevant Lines: 1940

💛 - Coveralls

@@ -152,15 +152,12 @@ export class WhitelistInputBlock extends React.Component {
/>
</div>
</div>
{whitelistElements && whitelistElements.map(e =>
{whitelist.length > 0 && whitelist.map((item, index) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure whitelist.length > 0 is not necessary, React will just ignore the resulting empty array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

const currentAddress = curr.addr.toLowerCase()
const previousAddress = prev.addr.toLowerCase()

return currentAddress > previousAddress ? -1 : currentAddress === previousAddress ? curr.stored ? 1 : -1 : 1
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

let whitelist = this.tiers[crowdsaleNum].whitelist.slice()
whitelist[whitelistNum].deleted = true
this.setTierProperty(whitelist, 'whitelist', crowdsaleNum)
const removedItem = this.tiers[crowdsaleNum].whitelist.splice(whitelistNum, 1)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs tests too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, and added test for the other modified/created methods

@fernandomg
Copy link
Contributor Author

@vbaranov conflict resolution done

@fvictorio
Copy link
Contributor

@vbaranov Can we merge this?

@vbaranov
Copy link
Collaborator

@fvictorio formally yes, but if you could give me some hours, I'll appreciate it. I plan to test it too soon.

@fvictorio
Copy link
Contributor

fvictorio commented Mar 14, 2018

Sure, no problem. I just want to merge as many PRs as possible so that I can start working on #536 with less risk.

@vbaranov vbaranov merged commit 95f57b3 into master Mar 14, 2018
@vbaranov vbaranov deleted the add-whitelist-address-in-manage-#596 branch March 14, 2018 17:23
@ghost ghost removed the awaiting for review label Mar 14, 2018
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.

(Bug) Modifiable crowdsale: incorrect behavior if user tries to add existing whitelisted address
4 participants