Skip to content
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

AuthenticatedUser.get_notifications() boolean query parameters always return true. #1671

Closed
newswangerd opened this issue Aug 23, 2020 · 4 comments · Fixed by #2001
Closed

Comments

@newswangerd
Copy link

Description

Setting all and participating filters don't filters doesn't do anything.

all=True and participating=True should filter notifications, but instead get_notifications always returns the full list of notifications.

Steps to reproduce

from github import Github
g = Github("secret")
user = g.get_user()

# all=False is broken
notifications = user.get_notifications(all=False)

# returns 371
print(notifications.totalCount)

notifications = user.get_notifications(all=True)

# returns 371
print(notifications.totalCount)
@s-t-e-v-e-n-k
Copy link
Collaborator

Based on the GitHub API docs and the code, we're sending the correct parameters, so we're entirely at the mercy of whatever GitHub sends back.

@s-t-e-v-e-n-k s-t-e-v-e-n-k changed the title get_notifications() boolean query parameters always return true. AuthenticatedUser.get_notifications() boolean query parameters always return true. Aug 30, 2020
@newswangerd
Copy link
Author

@s-t-e-v-e-n-k The github API seems to do the right thing. https://api.github.com/notifications?all=false only returns my unread notifications. I did notice that https://api.github.com/notifications?all=False (with the capital F) returns all my notifications.

It looks like the client sets the all param as a boolean:

params["all"] = all

Since python booleans use capital letters, I'm wondering if this getting set as ?all=False when the params get converted to a string. If that's the case, there might be other params that are broken as well, such as participating

@brighton1101
Copy link

I am running into this problem with participating and with all. I believe @newswangerd is correct in that the api does not assume False == false in the url params.

Am willing to put together a PR if you're interested in having this fixed.

@s-t-e-v-e-n-k
Copy link
Collaborator

Sounds like we need to say "true" if all else "false". I would welcome a PR! Make sure to include "Fixes #1671" in the commit message.

sshekdar-VMware added a commit to sshekdar-VMware/PyGithub that referenced this issue Jul 21, 2021
sshekdar-VMware added a commit to sshekdar-VMware/PyGithub that referenced this issue Jul 22, 2021
s-t-e-v-e-n-k pushed a commit that referenced this issue Oct 11, 2021
…r Notifications (#2001)

The parameters 'all' and 'participating' for AuthenticatedUser.get_notifications() should
be lower case strings, not textual forms of a boolean.

Fixes: #1671
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants