@pablobm commented on this pull request.
Something to mention in the description and the documentation: in
`storage.yml`, now we'll need `service: GpxS3`.
> + def put_object_request
+ @service.client.client.api_requests.find { |request|
request[:operation_name] == :put_object }
+ end
This name initially made me think that it issued a PUT request, but instead if
finds a recent one. How about a rename? With a slight implementation change to
match:
```suggestion
def find_latest_put_object_request
@service.client.client.api_requests.rfind { |request|
request[:operation_name] == :put_object }
end
```
> @@ -76,10 +76,33 @@ def tagstring=(s)
def file=(attachable)
case attachable
when ActionDispatch::Http::UploadedFile, Rack::Test::UploadedFile
- super(:io => attachable,
- :filename => attachable.original_filename,
- :content_type => content_type(attachable.path),
- :identify => false)
+ type = content_type(attachable.path)
+
+ if type == "application/gpx+xml"
```suggestion
if type == Mime[:gpx]
```
> + # Flag server_gzipped=true, so the download and xml_file know the
> server gzipped it.
+ super(:io => gz,
+ :filename => attachable.original_filename,
+ :content_type => "application/gpx+xml",
+ :metadata => { "custom" => { "server_gzipped" => true } },
+ :identify => false)
Perhaps clearer like this?
```suggestion
super(:io => gz,
:filename => attachable.original_filename,
:content_type => "application/gpx+xml",
:metadata => {
"custom" => {
# We gzipped it. The user did not send it gzipped already.
"server_gzipped" => true
}
},
:identify => false)
```
> @@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+module GpxDownloadMethods
+ extend ActiveSupport::Concern
+
+ private
+
+ # Send the stored trace file to the client. It can go back two ways.
+ # - If the store serves gzip (S3) and the client accepts it, redirect and
let the client unzip it.
+ # - Otherwise the server unzips it first and sends plain GPX.
+ def send_trace_file(trace)
+ if gzipped_by_server?(trace)
Is this method necessary any more? If I'm understanding correctly, nothing is
going to answer `true` to this question as we remove the metadata on upload. I
didn't notice initially, but then I was testing online and that's what I'm
seeing: files don't keep this piece of metadata. Which is fine, because the
knowledge of the `Content-Encoding` lives in object storage now.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/7124#pullrequestreview-4532570975
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/7124/review/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
