-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
feat: comment fee control #1768
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed a small thing regarding saloon items and one nitpick but other than that, this is fabulous!
But also want to confirm with @huumn the following:
I noticed that we still have sub.allowFreebies
in the schema with no way for founders to configure it. The option was removed from the frontend in #1333 but we kept it in the schema for backwards compatibility. The value is not consistent across territories in prod currently but that's okay since it's no longer used anywhere anyway.
However, reading the description of #1333, I think the intention was to only disable freebies for posts. Since the base cost of comments was always 1 sat, we didn't need to consider freebie comments at that time.
But since this PR changes the base cost for comments: do we still want to allow freebies if cost <= baseCost
? Or only if cost <= 1
?
I think we can ship this as-is (with cost <= baseCost
as part of the freebie condition) and see how stackers respond.1
edit: I also wonder if we should use the following UI:
instead of
patch
diff --git a/components/territory-header.js b/components/territory-header.js
index a53a99e8..8d64f014 100644
--- a/components/territory-header.js
+++ b/components/territory-header.js
@@ -57,13 +57,16 @@ export function TerritoryInfo ({ sub }) {
<span> on </span>
<span className='fw-bold'>{new Date(sub.createdAt).toDateString()}</span>
</div>
- <div className='text-muted'>
- <span>post cost </span>
- <span className='fw-bold'>{numWithUnits(sub.baseCost)}</span>
- </div>
- <div className='text-muted'>
- <span>reply cost </span>
- <span className='fw-bold'>{numWithUnits(sub.replyCost)}</span>
+ <div className='d-flex'>
+ <div className='text-muted'>
+ <span>post cost </span>
+ <span className='fw-bold'>{numWithUnits(sub.baseCost)}</span>
+ </div>
+ <span className='px-1'> \ </span>
+ <div className='text-muted'>
+ <span>reply cost </span>
+ <span className='fw-bold'>{numWithUnits(sub.replyCost)}</span>
+ </div>
</div>
<TerritoryBillingLine sub={sub} />
</CardFooter>
Footnotes
Thanks for the exhaustive review ek!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed in my previous review that this will also need the same check in the backend
Description
Adds the possibility to customize comment/replies fees for territory owners, addressing #1186
Screenshots
territory overview
territory edit
reply
Additional Context
As originally conceived, if you're not the OP, fee is bound to be multiplied x10 to avoid item spam (eg. 150 -> 1500 -> 15000 etc).
findRootItem
(line 16) needs to query item more than once to obtain the root in case of replying to another replyChecklist
Are your changes backwards compatible? Please answer below: Yes
On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
8, replied on bio (1 sat), items, multiple comments. Creation of items and bios follow their original behavior.
For frontend changes: Tested on mobile, light and dark mode? Please answer below: n/a
Did you introduce any new environment variables? If so, call them out explicitly here: No