Skip to content

Messages in past n days endpoint#789

Merged
jchristgit merged 6 commits into
mainfrom
messages-in-past-n-days-endpoint
Nov 20, 2022
Merged

Messages in past n days endpoint#789
jchristgit merged 6 commits into
mainfrom
messages-in-past-n-days-endpoint

Conversation

@wookie184
Copy link
Copy Markdown
Contributor

A GET endpoint that takes data from the body is a bit dodgy which made this a pain 😩 . Am open to any improvements, I considered using a POST but not sure it's worth changing at this point.

Closes #783

Takes multiple users for efficiency as we may want to calculate this for many users at once.
I really had to work against DRF to get this working. Using the validator manually here isn't ideal but I couldn't see an obvious better way without adding a bunch of boilerplate code. It seems to work.
@wookie184 wookie184 requested a review from jb3 as a code owner October 27, 2022 15:58
@netlify
Copy link
Copy Markdown

netlify Bot commented Oct 27, 2022

Deploy Preview for pydis-static ready!

Name Link
🔨 Latest commit 7b49952
🔍 Latest deploy log https://app.netlify.com/sites/pydis-static/deploys/637a2d683246740008371e9b
😎 Deploy Preview https://deploy-preview-789--pydis-static.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 27, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling 7b49952 on messages-in-past-n-days-endpoint into 42feef2 on main.

@wookie184 wookie184 added area: API Related to or causes API changes type: feature New feature or request priority: 2 - normal Normal Priority python Used by dependabot s: needs review Author is waiting for someone to review and approve labels Oct 27, 2022
@jchristgit
Copy link
Copy Markdown
Contributor

This was still being worked out re: conversation in #dev-core, right?

@jchristgit jchristgit self-assigned this Oct 27, 2022
@jchristgit jchristgit self-requested a review October 27, 2022 21:36
@wookie184
Copy link
Copy Markdown
Contributor Author

Yeah, I think I'll just change it to be a POST endpoint.

@wookie184
Copy link
Copy Markdown
Contributor Author

Should be ready for review now

@wookie184
Copy link
Copy Markdown
Contributor Author

Thoughts on changing this to return 0 for users with no messages?

I didn't do it originally because it would also return 0 for invalid IDs which could potentially be misleading, but now i'm thinking it would probably be more useful if it just returned 0 for the ID if nothing was found.

@shtlrs
Copy link
Copy Markdown
Contributor

shtlrs commented Nov 11, 2022

I guess we could have the user ids as comma-separated-values, but we'd be running the risk of having too long query strings :/
I think it's fine to have this as a POST endpoint, even though it "sounds" wrong.

The only way to make this a GET would be to fetch the data of one user, but it would basically be a waste of bandwidth.

I'm saying this because I don't know what was discussed in #dev-core, so just shooting out ideas.

@shtlrs
Copy link
Copy Markdown
Contributor

shtlrs commented Nov 11, 2022

Thoughts on changing this to return 0 for users with no messages?

I didn't do it originally because it would also return 0 for invalid IDs which could potentially be misleading, but now I'm thinking it would probably be more useful if it just returned 0 for the ID if nothing was found.

I think it would be a good idea to return 0 for users with no messages.

Currently, if at least one id is invalid, the request won't be successful and will return with the encountered errors.

image

Unless there's a detail that I'm missing

@wookie184
Copy link
Copy Markdown
Contributor Author

I was referring to the case where we can't tell if the ID is correct or not. For cases where it's not an integer or is negative the API user has clearly done something wrong so I think it makes sense to return an error response.

The issue is that with the current query we can't tell between users with no messages in that period and users that don't exist (but with the ID in the correct form). I think i'll change it so that does return 0 anyway though, as it's probably more useful.

@shtlrs
Copy link
Copy Markdown
Contributor

shtlrs commented Nov 12, 2022

I was referring to the case where we can't tell if the ID is correct or not. For cases where it's not an integer or is negative the API user has clearly done something wrong so I think it makes sense to return an error response.

The issue is that with the current query we can't tell between users with no messages in that period and users that don't exist (but with the ID in the correct form). I think i'll change it so that does return 0 anyway though, as it's probably more useful.

When would we have the case of a non existent ID ?
Because what I'm wondering is, if the user is a member of server but not found in the database, we would have a delta between the number of items returned by the server & the number of items requested.

And if the user is not in the database, then the query won't return any information about them since they don't exist.

What I'm thinking is, if we "desperately" want that information, we can have some sort of query post-processing before the API returns a response, and non existent user will have a value of -1 for example.

@wookie184
Copy link
Copy Markdown
Contributor Author

This is ready for review now.

What I'm thinking is, if we "desperately" want that information, we can have some sort of query post-processing before the API returns a response, and non existent user will have a value of -1 for example.

We don't need that information currently, so it's probably not worth overcomplicating it.

Copy link
Copy Markdown
Contributor

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legend man. LEGEND.

@jchristgit
Copy link
Copy Markdown
Contributor

This is what the American country has been looking forward to for years. As a single nation, we are finally able to provide a detailed and in-depth aggregation of information on American citizens, which further improves our commitment to a brighter future for our citizens. It is imperative to be aware of our environmental impact, and I am hereby signing this pull request into master.

Copy link
Copy Markdown
Contributor

@Xithrius Xithrius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no issues with the current implementation of this feature.

@jchristgit jchristgit merged commit 52c2439 into main Nov 20, 2022
@jchristgit jchristgit deleted the messages-in-past-n-days-endpoint branch November 20, 2022 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Related to or causes API changes priority: 2 - normal Normal Priority python Used by dependabot s: needs review Author is waiting for someone to review and approve type: feature New feature or request

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Endpoint for getting activity from metricity

5 participants