Skip to content

Conversation

watchtower314
Copy link
Contributor

@watchtower314 watchtower314 commented Apr 5, 2021

Issue #266

THIS HAS A FRONTEND PR TOO!
Tests are failing -> fixing them [wip]

Copy link

@alexander2001i alexander2001i left a comment

Choose a reason for hiding this comment

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

I'm not getting what this PR is trying to modify. Can you elaborate?

@watchtower314
Copy link
Contributor Author

watchtower314 commented Apr 6, 2021

@alexander2001i yeah, took me a few reads of the ticket to understand to motivation here 😅
so basically, when creating a common, it is required that there will be a minFeeToJoin (which must be at least $5). That means that users cannot join for free, so there's a new ui change that allows the common creator to decide if users can join for free
Screen Shot 2021-04-06 at 3 01 40
When the creator ticks the checkbox, we need to allow users to join without providing the minimum contribution.

If you have more questions/suggestions, let me know ☺️

Ticket: #266

@alexander2001i
Copy link

@watchtower314 No, I don't. Thanks for the explanation

links: yup.array(linkValidationSchema).optional()
links: yup.array(linkValidationSchema).optional(),

zeroContribution: yup.boolean().required(),

Choose a reason for hiding this comment

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

Maybe don't make it required, but rater if not provided set to false?


// Check if the request is funded with less than required amount
if (common.metadata.minFeeToJoin > payload.funding) {
if (common.metadata.minFeeToJoin > payload.funding && !common.metadata.zeroContribution) {

Choose a reason for hiding this comment

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

This is not correct, because if zero dollar contributions are allowed it will allow the user to create contribution, lower than the minimum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! I'll fix it, Thank you! :)

Choose a reason for hiding this comment

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

You are welcome 😄

Copy link

@lpetkov-sw lpetkov-sw left a comment

Choose a reason for hiding this comment

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

Hey @watchtower314 as it's described in the ticket you have removed the Safety Period feature, but here I don't see any logic for that. I think at least you have to check the tests and the validation for the common creation and exclude/remove any logic about that field.

@watchtower314
Copy link
Contributor Author

Hey @watchtower314 as it's described in the ticket you have removed the Safety Period feature, but here I don't see any logic for that. I think at least you have to check the tests and the validation for the common creation and exclude/remove any logic about that field.

So I haven't removed the safety period, just forced it to be today, and we are still sending it from the frontend, should I remove it altogether?


const invalidJoinDataZeroContribution = (commonId: string) => ({
commonId,
description: 'I wanna be a part, but pay less that $5',
Copy link

@lpetkov-sw lpetkov-sw Apr 8, 2021

Choose a reason for hiding this comment

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

Typo here: that -> than But no important at all :D

@lpetkov-sw
Copy link

Hey @watchtower314 as it's described in the ticket you have removed the Safety Period feature, but here I don't see any logic for that. I think at least you have to check the tests and the validation for the common creation and exclude/remove any logic about that field.

So I haven't removed the safety period, just forced it to be today, and we are still sending it from the frontend, should I remove it altogether?

@alexander2001i What's your opinion ?

@alexander2001i
Copy link

Yes, It thing it will be the best to not have it at all rather than having a dummy value

@alexander2001i alexander2001i merged commit cde415b into main Apr 9, 2021
@alexander2001i alexander2001i deleted the CM-266-common-creation-min-contribution branch April 9, 2021 05:09
alexander2001i pushed a commit that referenced this pull request May 7, 2021
Co-authored-by: Anton <anton@Antons-Mac-mini.local>
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