chore: refactor controllers #461
Conversation
| dbSvc db.DBService | ||
| cfg config.Config | ||
| svc service.Service | ||
| permissionsSvc permissions.PermissionsService |
There was a problem hiding this comment.
We are adding this svc just to make use of the GetBudgetUsage function, moved GetStartOfBudget to utils folder and after that it is just a one line sql query which I moved to this file directly.
There was a problem hiding this comment.
I would not like to duplicate that SQL query, this is the core logic to get the budget usage which is also used to see if an app has permission to make a payment
There was a problem hiding this comment.
Then we might have to add the controllersSvc just for that function, should I do it?
|
|
||
| assert.Equal(t, 2, len(responses)) | ||
| for i := 0; i < len(responses); i++ { | ||
| assert.Equal(t, "123preimage", responses[i].Result.(payResponse).Preimage) | ||
| assert.Equal(t, "123preimage", responses[i].Result.(controllers.PayResponse).Preimage) |
There was a problem hiding this comment.
I don't think we should move the tests to a new package, now we have to make models public just for the tests - this is why I put them side by side with the code. A lot of other projects do this too.
| @@ -20,6 +19,10 @@ import ( | |||
| ) | |||
|
|
|||
| func (svc *nip47Service) HandleEvent(ctx context.Context, sub *nostr.Subscription, event *nostr.Event, lnClient lnclient.LNClient) { | |||
| if !svc.controllersService.HasLNClient() { | |||
There was a problem hiding this comment.
this feels like a hack, can we pass the LNClient to the individual controller handler methods?
| @@ -77,7 +77,7 @@ func (notifier *Nip47Notifier) notifySubscribers(ctx context.Context, notificati | |||
| notifier.db.Find(&apps) | |||
|
|
|||
| for _, app := range apps { | |||
| hasPermission, _, _ := notifier.permissionsSvc.HasPermission(&app, permissions.NOTIFICATIONS_PERMISSION, 0) | |||
| hasPermission, _, _ := notifier.controllersSvc.HasPermission(&app, controllers.NOTIFICATIONS_PERMISSION, 0) | |||
There was a problem hiding this comment.
I think this is coupling notifications to controllers - these are two separate things and should not be related
|
@im-adithya @bumi I feel like we unfortunately went a bit backwards on this PR. I suggest we make the minimal change to create a common interface for the controller methods and then consider the other stuff as a follow-up. Does that sound ok? |
|
@im-adithya I talked with @bumi and we thought for now it's not worthwhile to make this change as it's opening up some other issues (as per the comments). For now we'd like to close this one and focus on some more other things (changing the authentication to not use a session cookie for example). I will create a new issue keep track of this though and link to this PR. |
Removes
PermissionsServiceand addsControllerService