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

Feature/tdbytes #96

Merged
merged 5 commits into from
May 27, 2024
Merged

Feature/tdbytes #96

merged 5 commits into from
May 27, 2024

Conversation

otytlandsvik
Copy link
Contributor

💎 tdBytes pages and kiosk admin role

  • Added endpoints for kiosk product suggestions

    • Add suggestion
    • Get all suggestions
    • Delete suggestion
  • Also added a new kiosk admin role to allow non-admins to view suggestions

  • 🐳 removed obsolete version tags from docker compose files

Copy link

@stiansolli stiansolli left a comment

Choose a reason for hiding this comment

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

Looks good! Will assume all tests pass without testing myself

Copy link
Member

@FredrikMorstad FredrikMorstad left a comment

Choose a reason for hiding this comment

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

LGTM, and nice that you added tests 🙌 I left one comment on wether or not a user suggestion should be capped but that is not very plausible that users spam their suggestions

app/api/kiosk.py Outdated
Comment on lines 20 to 21
if member is None:
raise HTTPException(500)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe 404 here, not really an internal server error IMO


class KioskSuggestion(KioskSuggestionPayload):
id: str
member: Member
Copy link
Member

Choose a reason for hiding this comment

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

Just a question, do we need to know the member adding a suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a requirement, and only admins are allowed to view this data as it can be sensitive. However, I thought it was a nice utility if admins want to ask a person what they mean by certain suggestions, or hold users accountable for spamming or other abuse.



@router.post("/suggestion")
def add_suggestion(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not relevant now, but we should maybe look to guard against users spamming suggestions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, many endpoints could use some rate limiting mechanism

@otytlandsvik otytlandsvik merged commit 380f0c4 into master May 27, 2024
1 check passed
@otytlandsvik otytlandsvik deleted the feature/tdbytes branch May 27, 2024 20:26
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.

3 participants