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

Templates: add Open Enterprise #1096

Merged
merged 13 commits into from Oct 24, 2019

Conversation

@ottodevs
Copy link
Contributor

ottodevs commented Sep 27, 2019

Add Open Enterprise template

Update: Reviewed and rebased from dev accounting on latest changes. I would say is author-ready

Intro

Builds on #1091, the main commit for this addition is 1779d27 (previous commits are result of rebasing)

Dear reviewers, please let me know if this PR makes sense to be merged as it is, with the discussions setting disabled (code commented) for now, or would you rather to wait for the Forwarder API to be merged and the apps already published into the hatch APM?

Anyway I would appreciate any feedback in the meanwhile 😇

Changes included

  • Add open-enterprise template to templates index
  • Add DotVotingScreen component to screens [1][2]
  • Add header and icon images
  • Add template definition and deployment file for open enterprise
  • Disable discussions settings since it will not be ready initially
  • Set labels and default values as per specs. Unlink sliders
  • Add KnownAppBadge for Voting and DotVoting
  • Updated copy for Dot Voting template screen (co-authored-by @stellarmagnet)
  • Fix formatting
  • Copycat from @bpierre appLabel prop adding
  • Update transactions params, enforce multiple tx

Squashed into a single commit for a cleaner history

Credits

Kudos to @jayalaves for the great Open Enterprise asset designs 🏆


Footnotes

[1]: Could be eventually merged with Voting screen, for now I separated the components, as DotVoting has the additional Discussions integration checkbox, the sliders are not linked, and defaults are different. Also Dot Votes use minimum participation instead of minimum approval %
[2]:Requires Duration component added in ad7423f so I rebased templates-various-fixed branch here
@ottodevs

This comment has been minimized.

Copy link
Contributor Author

ottodevs commented Sep 27, 2019

Do you think 1 minute is a safe minimum for Dot Votes duration?

@ottodevs ottodevs force-pushed the AutarkLabs:template-open-enterprise branch from a79b3ad to 7045292 Sep 27, 2019
@ottodevs ottodevs force-pushed the AutarkLabs:template-open-enterprise branch from 7045292 to bdace18 Sep 27, 2019
@ottodevs ottodevs force-pushed the AutarkLabs:template-open-enterprise branch from 0e6c825 to bf6a51a Oct 3, 2019
@ottodevs ottodevs force-pushed the AutarkLabs:template-open-enterprise branch from bf6a51a to aee91c9 Oct 3, 2019
@ottodevs ottodevs force-pushed the AutarkLabs:template-open-enterprise branch from aee91c9 to d8d85bc Oct 3, 2019
@ottodevs ottodevs force-pushed the AutarkLabs:template-open-enterprise branch from d8d85bc to 204f76c Oct 3, 2019
@now now bot requested a deployment to staging Oct 3, 2019 Pending
@ottodevs ottodevs force-pushed the AutarkLabs:template-open-enterprise branch from 204f76c to 1779d27 Oct 3, 2019
@AquiGorka

This comment has been minimized.

Copy link
Contributor

AquiGorka commented Oct 4, 2019

Looking :snazzy: 🎉
Question: I was not able to create the organization, got this error:

image

Have you seen this before? Any thoughts on why it might be failing for me?

@stellarmagnet

This comment has been minimized.

Copy link
Contributor

stellarmagnet commented Oct 5, 2019

@AquiGorka apologies if it wasn't clear -- we wanted to get everything reviewed that we had so far and the missing piece of the puzzle was doing the final connection.

Will resolve your comments and let you know when the rest of it is ready for a final run through. Thank you!

@ottodevs

This comment has been minimized.

Copy link
Contributor Author

ottodevs commented Oct 8, 2019

Looking :snazzy: 🎉
Question: I was not able to create the organization, got this error:

image

Have you seen this before? Any thoughts on why it might be failing for me?

We need to publish the template to APM to fix that one, the task is still pending for us, as Yalda explained, will be ready soon!

Copy link
Member

bpierre left a comment

Hi @ottodevs, it’s looking good!

We should also add the apps icons into the KnownAppBadge component, to have them appear on this screen:

Dear reviewers, please let me know if this PR makes sense to be merged as it is, with the discussions setting disabled (code commented) for now, or would you rather to wait for the Forwarder API to be merged and the apps already published into the hatch APM?

I would remove this part for now, until the Forwarder API is ready.

ottodevs added 3 commits Oct 15, 2019
… params
@ottodevs ottodevs force-pushed the AutarkLabs:template-open-enterprise branch from c75a3f2 to dc14c6c Oct 23, 2019
@ottodevs

This comment has been minimized.

Copy link
Contributor Author

ottodevs commented Oct 23, 2019

Rebased latest dev and addressed @sohkai comments where possible! 👍

@ottodevs ottodevs force-pushed the AutarkLabs:template-open-enterprise branch from dc14c6c to 1c15c0c Oct 23, 2019
@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Oct 23, 2019

@ottodevs Would be great to avoid the rebase next time, and just merge :). It makes it a bit easier to review the latest changes in a PR.

sohkai added 2 commits Oct 23, 2019
…ach template
@sohkai
sohkai approved these changes Oct 23, 2019

const DEFAULT_SUPPORT = 0
const DEFAULT_QUORUM = 15
const DEFAULT_DURATION = MINUTE_IN_SECONDS

This comment has been minimized.

Copy link
@stellarmagnet

stellarmagnet Oct 23, 2019

Contributor

This defaults to one minute. I think we should make this DAY_IN_SECONDS

const adjustedDvSupport = ONE_PERCENT.mul(new BN(dvSupport))

// The min value for quorum must be 1 or greater
const adjustedDvQuorum = dvQuorum.isZero()

This comment has been minimized.

Copy link
@stellarmagnet

stellarmagnet Oct 23, 2019

Contributor

I think this line is making it throw an error. After I click "Launch organization", I get:

t.isZero is not a function
in Unknown
in div
in t
in Onboarding___StyledDiv3
in div
in t
- Change default dot voting duration setting to one day
- Fix comma spacing on the template description
- Fix isZero not found error
export default {
id: 'open-enterprise-template.aragonpm.eth',
name: 'Open Enterprise',
header,

This comment has been minimized.

Copy link
@ottodevs

ottodevs Oct 23, 2019

Author Contributor

But off-chain check does not make lot of sense for me when the goal is to create an on-chain template instance, I am probably missing something here

@@ -32,13 +32,17 @@ function reduceFields(fields, [field, value]) {
if (field === 'quorum') {
return {
...fields,
// 100% quorum is not possible, but any adjustments necessary should be handled in the
// templates themselves

This comment has been minimized.

Copy link
@ottodevs

ottodevs Oct 23, 2019

Author Contributor

Just a warn here, correct me if wrong:

If template themselves means the contract, it might represent a poor UX, since the template will fail at tx level when it could be prevented with basic onboarding checks that could effectively drive the user on the cause and solution. Specially for example on multiple tx process: A succesful first tx followed by a reverted one because the Voting settings were wrong seems awful

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 23, 2019

Member

If template themselves means the contract

I meant checking for this in the template's frontend implementation. The template contract will fail, because it cannot initialize the Voting contract with those parameters. Will update the wording.

…ntends
@sohkai sohkai merged commit 79df40e into aragon:master Oct 24, 2019
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
now Deployment has completed
Details
@sohkai sohkai deleted the AutarkLabs:template-open-enterprise branch Oct 24, 2019
@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Oct 24, 2019

@all-contributors please add @ottodevs for code :)

@allcontributors

This comment has been minimized.

Copy link
Contributor

allcontributors bot commented Oct 24, 2019

@sohkai

I've put up a pull request to add @ottodevs! 🎉

@aragon aragon deleted a comment from allcontributors bot Oct 24, 2019
chadoh added a commit to AutarkLabs/aragon that referenced this pull request Oct 24, 2019
* 'master' of github.com:aragon/aragon:
  docs: add adekbadek as a contributor (aragon#1144)
  docs: add ottodevs as a contributor (aragon#1143)
  chore: update @aragon/wrapper (aragon#1142)
  Templates: add Open Enterprise (aragon#1096)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.