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: public uploads #18295

Closed
wants to merge 9 commits into from
Closed

feat: public uploads #18295

wants to merge 9 commits into from

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Oct 31, 2023

Problem

To support the In-App Video Recording hackathon in Bologna we needed a way to allow attachments to be uploaded publicly.

We have no intention of merging this but leaving it here for the future traveller who might want to explore the approach in future. I will leave comments to discuss some difficulties / considerations faced along the way

Changes

  • Introduce an AttachmentsViewSet that creates an UploadedMedia object under the hood
  • Add a new "Add to notebook" column to the DataTable
  • Add new notebooks node to support attachments
Feedback Index Page Created Notebook
Screenshot 2023-10-31 at 10 03 00 Screenshot 2023-10-31 at 10 03 15

Not considered

  • How would you mark items as done?

How did you test this code?

Some minor tests but would need a lot more

try:
file = request.data["file"]

if file.size > (FOUR_MEGABYTES * 20):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arbitrary limit

@@ -125,11 +125,10 @@ def format_paginated_url(request: request.Request, offset: int, page_size: int,

def get_token(data, request) -> Optional[str]:
token = None
if request.method == "GET":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mobile SDK was passing the api_key as a query param but in a POST request. Unsure if this "GET" condition is necessary but we should consider removing it in future

# Throttle class that's very aggressive on a publicly accessible endpoint from the clients
# Intended to block sustained bursts of requests, per team
scope = "public_upload"
rate = "6/hour"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arbitrary

@daibhin daibhin closed this Oct 31, 2023
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

3 snapshot changes in total. 0 added, 3 modified, 0 deleted:

  • chromium: 0 added, 3 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

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