-
Notifications
You must be signed in to change notification settings - Fork 150
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
Change: Extends allNotifications with optional filters #3740
base: main
Are you sure you want to change the base?
Conversation
6876943
to
e2c246d
Compare
Just re-based and pushed (didn't change any code) before finalising review |
e2c246d
to
c90f58b
Compare
This works fine, but I'm wondering if this is the best approach to this now. The With organizations also being a thing, the likelihood of someone using this that isn't a platform-owner is probably low. I can't recall if there was a specific reason for these changes or not though, but maybe we can get away without this change for now? |
It was just so we had a way to retrieve unassigned notifications, right now once created they're essentially lost to the void until assigned to a project. I think there were some commands in the cli where this might be useful, but not super necessary. Happy to proceed without it for now though. |
Ahh that's right. Yeah, I think it might need some adjustments, so maybe we can bump this to 2.22.0 instead. I'll add some review comments with some suggested changes. |
General Checklist
Database Migrations
Extends
allNotifications
to have an optionalname
&type
input.