-
Notifications
You must be signed in to change notification settings - Fork 195
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: set allowance per provider #2776
Conversation
@@ -806,6 +806,7 @@ export interface Blocklist extends DbBlocklist {} | |||
|
|||
export interface DbAllowance { | |||
createdAt: string; | |||
enabledFor?: string[]; |
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.
what about providers
? is there any downside of this name? I feel like it's more specific
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.
allowance are set not just by provider. but also during the payments. for eg. while doing a lnurl-pay to the website, in future we might add some other entries there as well, isn't it good to keep it generalized. so that it can hold any kind of entries
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.
@bumi @reneaaron do you have any thoughts here? currently we have enable screens for providers
@rolznz do you have idea about that one failing test. i am still unable to get why is that failing? |
@pavanjoshi914 it seems you defined the array wrong in the dexie schema: https://dexie.org/docs/MultiEntry-Index (changing it to |
cleanup prompt logic to set EnabledFor use set to do logical operations and convert back to array to avoid tertiary operator
I've tested it and it seems to work well! The allowance screen hasn't been changed, I think it would it be good to show which providers have been enabled there. Would it be good to do this as a follow up PR? |
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.
tACK, I think we need another PR to display the enabled providers in the allowance screen and on the connected sites page.
Best to get a review from @reneaaron or @bumi though before merging
Describe the changes you have made in this PR
set allowance per provider
Link this PR to an issue [optional]
Fixes #2758
Type of change
(Remove other not matching type)
feat
: New feature (non-breaking change which adds functionality)Screenshots of the changes [optional]
2023-09-27.16-28-12.mp4
How has this been tested?
by enabling allowance per provider
Checklist