Skip to content

removed return error on 0 threshold logic#453

Merged
0xmovses merged 2 commits intomainfrom
rich/cas-509
Sep 14, 2022
Merged

removed return error on 0 threshold logic#453
0xmovses merged 2 commits intomainfrom
rich/cas-509

Conversation

@0xmovses
Copy link
Copy Markdown
Contributor

@0xmovses 0xmovses commented Sep 14, 2022

•Removes controller level logic which returned an error if proposal threshold was set to 0

Notes
This is a quick fix to allow for community creation when onlyAuthorsToSubmitProposals is true, as the old code would make the backend throw an error here.

Proper fix is, to write logic at the helper level that first:
checks if onlyAuthorsToSubmitProposals is true.
If yes, ensure that proposalThreshold >= 1

@linear
Copy link
Copy Markdown

linear Bot commented Sep 14, 2022

CAS-509

Copy link
Copy Markdown
Contributor

@germanurrus germanurrus left a comment

Choose a reason for hiding this comment

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

Tested locally and works fine

Comment on lines -362 to -365
propThreshold, err := strconv.ParseFloat(*payload.Proposal_threshold, 64)
if err != nil {
respondWithError(w, http.StatusBadRequest, "Error Converting Proposal Threshold to Float.")
}
Copy link
Copy Markdown
Contributor

@germanurrus germanurrus Sep 14, 2022

Choose a reason for hiding this comment

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

maybe we can keep this check

@jbluks
Copy link
Copy Markdown
Collaborator

jbluks commented Sep 14, 2022

We should maybe ask @tuffon about this, since these changes basically revert this commit: 5fe47b3

Unless we know that we want to allow for 0 as a valid proposal threshold, and it is the front-end that should control defaults for different scenarios.

@0xmovses
Copy link
Copy Markdown
Contributor Author

Here's the original ticket https://linear.app/dappercollectives/issue/CAS-414. @jbluks yeah it was being set as 0 before if onlyAuthorsToSubmitProposals was set to true, so I would say valid for this case.

@0xmovses 0xmovses merged commit 37477e3 into main Sep 14, 2022
@0xmovses 0xmovses deleted the rich/cas-509 branch September 14, 2022 16:28
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.

3 participants