Minor confusion with user feed endpoint docs

[reallytiredofclowns]

Hi,

Should we be reporting API questions in the docs repo or on the Discuit repo? In working with the API, I've noticed some issues where the old documentation didn't seem to match up with what was actually returned.

One example is the user feed endpoint.

The descriptive text says, "Feeds of banned users can only be viewed by admins. For anyone else, a 403 error is returned." However, the chart mentions 401 should be returned for banned users.

The error code returned seems to even differ depending on the request's logged-in status. Test case: https://discuit.net/api/users/Spruce/feed

I get {"status":403,"code":"not_admin","message":"You are not an admin."} when logged in and {"status":401,"code":"user_banned","message":"User is banned."} when logged out.

From the text, it feels like 403 should be returned whether logged in or not, if I'm not an admin.

Also, I can go through some of my other notes and point out other API issues, but I don't know Go, so can't tell in general if something is a documentation issue or a code issue. Were you expecting people to actually do pull requests to help update the documentation, or is opening an issue enough?

[noClaps]

Should we be reporting API questions in the docs repo or on the Discuit repo? In working with the API, I've noticed some issues where the old documentation didn't seem to match up with what was actually returned.

If it's an issue with how the endpoint has been documented, then the issue should be opened here. If it's a bug with the endpoint returning something that it shouldn't, or some other issue with the code, then it should be opened in the Discuit repo.

This seems like a case of improper documentation, although it would be helpful to have @previnder clarify why the endpoint behaves this way so that it can be documented.

Also, I can go through some of my other notes and point out other API issues, but I don't know Go, so can't tell in general if something is a documentation issue or a code issue. Were you expecting people to actually do pull requests to help update the documentation, or is opening an issue enough?

That's completely fine. I'd actually prefer it if you opened an issue first explaining what's missing/incorrect about the docs, what the correct information is (if you know), and then say whether you'd be willing to make a PR for it or not. If you're not sure, just opening an issue is perfectly fine! I'd appreciate if you opened separate issues for each API issue you found, since it would make it easier to keep track of and refer back to in the future.

[Codycody31]

From the text, it feels like 403 should be returned whether logged in or not, if I'm not an admin. This would make sense, but from the code standpoint here is how the logic is:

if user.Banned { // Forbid viewing profile of banned users except for admins.
  if !r.loggedIn { <-- Check if the person making the user is NOT logged in
	  return &httperr.Error{ <-- If so just inform the user that the person they are trying to see the feed of is banned
		  HTTPStatus: http.StatusUnauthorized,
		  Code:       "user_banned",
		  Message:    "User is banned.",
	  }
  }
  viewer, err := core.GetUser(r.ctx, s.db, *r.viewer, nil)
  if err != nil {
	  return err
  }
  if !viewer.Admin { <-- if user is not an admin then don't let them see it
	  return httperr.NewForbidden("not_admin", "You are not an admin.")
  }
}

Though I do find it interesting on the logic here, but I think it partially makes sense as we don't want to make a unneeded call the DB to check if the user is in fact a admin, and it is easier to just inform the requester that the user's feed they wish to see was banned. And makes it the error a bit more understandable rather then a random person who is not logged in just getting aYou are not an admin..

Though this is just conjecture as I don't know the actually idea behind the logic (sorry for the dump).

[noClaps]

Though I do find it interesting on the logic here, but I think it partially makes sense as we don't want to make a unneeded call the DB to check if the user is in fact a admin, and it is easier to just inform the requester that the user's feed they wish to see was banned. And makes it the error a bit more understandable rather then a random person who is not logged in just getting aYou are not an admin..

I think that makes sense. The endpoint already lists the possible errors here, so I don't think there's any changes needed. We still need to document the possible errors, but that's a different thing. I'll leave this issue open until the relevant errors for this have been documented. I'll leave the needs clarification label in case @previnder wants to add something.