-
Notifications
You must be signed in to change notification settings - Fork 17
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
Fix returning deleted admin #243
Conversation
- the `findAdmin` method didn't filter for `isDeleted`, yet
7789de2
to
ddfc699
Compare
->getQuery() | ||
->getResult()[0]; |
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.
I know I'm late but I wonder why this didn't have a limit applied to it 🤔
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.
I mean how many admins will an instance have :D
I'd say most instances are running with exactly one admin
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.
hmm, i wonder what happens where there's more than 1, though... it might hose whatever is expecting only 1 like assigning a magazine owner? you're probably right about there usually only being 1 admin, but these edge cases might come back to bite later.
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.
It's not important, I just feel like if you already know the limitations you expect, it's good to declare, so I might want to add ->setMaxResults(1)
to this or something, but also like I said not important
this is fine, the function always returns 1 result, the sql might just return more rows than you use, really not a big deal
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.
I mean how many admins will an instance have :D I'd say most instances are running with exactly one admin
Mine has three
well it just takes the first onw, so the edge case is kinda solved. The whole owner thing justneedstobe removed... Maybe we could say the mod that has been a mod the longest is the "owner", but I digress 😅
Am 11. November 2023 01:39:15 MEZ schrieb debounced ***@***.***>:
…
@nobodyatroot commented on this pull request.
> ->getQuery()
- ->getResult()[0];
hmm, i wonder what happens where there's more than 1, though... it might hose whatever is expecting only 1 like assigning a magazine owner? you're probably right about there usually only being 1 admin, but these edge cases might come back to bite later.
--
Reply to this email directly or view it on GitHub:
#243 (comment)
You are receiving this because you modified the open/close state.
Message ID: ***@***.***>
|
findAdmin
method didn't filter forisDeleted
, yetFix #67