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

feat: Implement GET endpoint to fetch URLs for a given user and admin access #60

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rahul-MyGit
Copy link

PR added:

  • Fetch all URLs created by current user
  • When given Admin API Key, fetch all URLs (any user) and filter by ?userid=xxx

Resolve #[#59]

Checklist before requesting a review

  • I assure there is no similar/duplicate pull request regarding same issue

@@ -22,6 +24,7 @@ func UrlsRoute() func(router fiber.Router) {

return func(router fiber.Router) {
router.Get("/", getAllUrls)
router.Get("/admin", security.MandatoryAdminApiKeyAuthMiddleware, getAllUrlsAdmin)
Copy link
Owner

Choose a reason for hiding this comment

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

i was preferring not to have /admin/urls or /urls/admin separately but simply use the /urls endpoint with the X-API-Key to work for admins.

Copy link
Author

Choose a reason for hiding this comment

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

Sure I'll do that

"math"
"math/rand"
"onepixel_backend/src/db"
"onepixel_backend/src/db/models"
"onepixel_backend/src/utils"
"onepixel_backend/src/utils/applogger"
"sync"

Copy link
Owner

Choose a reason for hiding this comment

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

why the juggling of imports? needed?

Copy link
Author

@rahul-MyGit rahul-MyGit Aug 4, 2024

Choose a reason for hiding this comment

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

Whenever I save this file, it automatically juggles. As there are no spaces between them, it's prolly done by go itself.

Copy link
Owner

Choose a reason for hiding this comment

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

ah ok that's fine then

func getAllUrls(ctx *fiber.Ctx) error {
return ctx.SendString("GetAllUsers")
apiKey := ctx.Get("X-API-Key")
Copy link
Owner

Choose a reason for hiding this comment

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

can we fail fast if neither JWT nor API Key is given?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll do the same

@@ -106,3 +107,12 @@ func (c *UsersController) VerifyEmailAndPassword(email string, password string)
}
return user, nil
}

func (c *UrlsController) GetUrlsByUserId(userId uint64) ([]models.Url, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

getting URLs should not be happening in Users controller, it should happen in Urls Controller only

userId = &user.ID
}

var urls []models.Url
Copy link
Owner

Choose a reason for hiding this comment

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

what is the difference in the if condition here ?

and why do we want it to be different ?
if the userId came from LOCALS or it came from query (in case of admin) how does it differ? the logic for fetching the URLs will be exactly same in both those cases.

Copy link
Author

@rahul-MyGit rahul-MyGit Aug 7, 2024

Choose a reason for hiding this comment

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

Sir, top level JWT/X-API-KEY if() check is wrong.

  • But in isAdmin; here we can't check userId at LOCALS (and in query the userId will be string so we've to convert it) and vise-versa in users case(here user.ID is integer). So that's why it was a good approach to separate them using if-else:

  • what we can add now is"

    • If userIdStr is empty or invalid, userId is explicitly set to nil.
    • have a new if-else check after this completes which will check :
    • if userId != null then GetUrlsByUserId(*userId) //works for both admin and users
      else GetAllUrls(nil) //only work for admin to get all urls.
  • This will maintain the userId consistency and also it checks the null values.

@rahul-MyGit
Copy link
Author

@championswimmer is this code works ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants