Introduce the Dummy. #70

dixpac · 2017-07-23T12:16:45Z

First step:

Convert controller tests to integration tests
Move storage configurations to dummy
Remove unnecessary requires

r? @dhh
We could slim down a bit this dummy, to remove rails components that are not used at the moment, ex: ActionCable ?

dixpac · 2017-07-23T12:19:51Z

test/test_helper.rb

 require "yaml"
 SERVICE_CONFIGURATIONS = begin
-  YAML.load_file(File.expand_path("../service/configurations.yml", __FILE__)).deep_symbolize_keys
+  YAML.load_file(File.expand_path("../dummy/config/storage_services.yml", __FILE__)).deep_symbolize_keys


I've moved the configurations inside the dummy, otherwise dummy app will fail on the boot. Also, because of this current Travis tests will fail

dhh · 2017-07-24T19:00:36Z

I'd welcome slimming this down. There are a ton of files that aren't relevant. But if you could otherwise bring in line with master, we can merge. Definitely would like to switch to integration tests.

dixpac · 2017-07-25T11:58:39Z

test/controllers/disk_controller_test.rb

-    assert_equal "inline; filename=\"#{@blob.filename}\"", @response.headers["Content-Disposition"]
+    get rails_disk_blob_url(
+      filename: "hello.txt",
+      content_type: @blob.content_type,


Here I'd to pas content-length in order for tests to pass. @dhh I've noticed that you changed the approach Here.

Not sure if your intention was to pass content_type of to figure him out automatically ?

Plan is to pass it in to the services, since they don't store it, and the blob has it.

dixpac · 2017-07-25T11:59:43Z

test/models/blob_test.rb

@@ -35,6 +35,7 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase

  private
    def expected_url_for(blob, disposition: :inline)
-      "/rails/active_storage/disk/#{ActiveStorage.verifier.generate(blob.key, expires_in: 5.minutes, purpose: :blob_key)}/#{blob.filename}?disposition=#{disposition}&content_type=#{blob.content_type}"
+      query_string = { content_type: blob.content_type, disposition: disposition }.to_param


Rails encodes the url so I'd to encode the query_string in order to match it sucessfully

dixpac · 2017-07-25T12:00:12Z

test/service/disk_service_test.rb

@@ -6,7 +6,7 @@ class ActiveStorage::Service::DiskServiceTest < ActiveSupport::TestCase
  include ActiveStorage::Service::SharedServiceTests

  test "url generation" do
-    assert_match /rails\/active_storage\/disk\/.*\/avatar\.png\?disposition=inline/,
+    assert_match /rails\/active_storage\/disk\/.*\/avatar\.png\?.+disposition=inline/,


Changed a regex a bit because of the url encoding

dixpac · 2017-07-25T12:03:15Z

@dhh I rebase it from the latest. Commit is a bit big(sorry) so I've added few comments on what I've changed from the first PR.

In the next commits/days I will try to slim this down

dixpac · 2017-07-26T09:20:59Z

@dhh I slimmed this quiet a bit (maybe too much) by removing all rails components we are not using at the moment.

There is one more approach I've tried and that is to slim all the way to the smallest app possible, take a look commit and here small app
(this has just a few files)

What approach do you think is better ?

dhh · 2017-07-26T14:51:30Z

test/controllers/disk_controller_test.rb

    assert_equal "text/plain", @response.headers["Content-Type"]
  end

  test "sending blob as attachment" do
-    get :show, params: { filename: @blob.filename, encoded_key: ActiveStorage.verifier.generate(@blob.key, expires_in: 5.minutes, purpose: :blob_key), disposition: :attachment }
-    assert_equal "attachment; filename=\"#{@blob.filename}\"", @response.headers["Content-Disposition"]
+    get rails_disk_blob_url(


Reminds me that now this should be rails_disk_service_url since the DiskController no longer deals with blobs, but only with keys.

dhh · 2017-07-26T14:53:09Z

I think it's good with the smallest possible app. We're basically just setting all this stuff up to be able to use the engine initialization and integration tests properly.

dhh · 2017-07-26T14:53:26Z

(Needs some rebasing after George merged the direct upload feature for the disk controller).

dixpac · 2017-07-26T17:01:24Z

@dhh I've rebased and added small dummy app

kaspth · 2017-07-26T17:34:30Z

Won't we just need to add back Action View and Sprockets when the direct upload JS arrives and has to be integration/system tested?

dixpac · 2017-07-26T17:48:26Z

@kaspth yes we would! That was my initial approach just to remove action cable and action mailer and leave other components in there (for the future). But, there were was concerned with the size of the dummy so I just created it as small as "possible", to cover current use.

The full dummy app would be most flexible/easy for the future use of testing.

dhh · 2017-07-26T18:43:43Z

Would probably be good to get AV and Sprockets back in, yeah, because @javan is just about ready with his direct upload JS PR. Thanks for working on this, Dino!

…

On Wed, Jul 26, 2017 at 12:48 PM, Dino Maric ***@***.***> wrote: @kaspth <https://github.com/kaspth> yes we would! That was my initial approach just to remove action cable and action mailer and leave other components in there (for the future). But, there were was concerned with the size of the dummy so I just created it as small as "possible", to cover current use. The full dummy app would be most flexible/easy for the future use of testing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtYsOuj7dhZuQ0qQI5AugC__NaXaXks5sR3vqgaJpZM4Ogb-S> .

dixpac · 2017-07-26T19:06:47Z

No problem! Will get them back :) On Jul 26, 2017 8:43 PM, "David Heinemeier Hansson" < notifications@github.com> wrote:

…

Would probably be good to get AV and Sprockets back in, yeah, because @javan is just about ready with his direct upload JS PR. Thanks for working on this, Dino! On Wed, Jul 26, 2017 at 12:48 PM, Dino Maric ***@***.***> wrote: > @kaspth <https://github.com/kaspth> yes we would! That was my initial > approach just to remove action cable and action mailer and leave other > components in there (for the future). But, there were was concerned with > the size of the dummy so I just created it as small as "possible", to cover > current use. > > The full dummy app would be most flexible/easy for the future use of > testing. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#70 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ AAAKtYsOuj7dhZuQ0qQI5AugC__NaXaXks5sR3vqgaJpZM4Ogb-S> > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHFVJe-8EB1mhaAIcUPWk6tB7wPgZ3Yiks5sR4jkgaJpZM4Ogb-S> .

dhh · 2017-07-26T19:40:41Z

There are a couple of conflicts against master, btw.

* Convert controller tests to integration tests * Move storage confogurations to dummy * Remove unecessary requires

dixpac · 2017-07-27T08:22:51Z

I've add back ActionView, Sprockets.... Now the dummy is larger because there is not a lot we can remove. I've removed ActionCable, ActionMailer and few config files.

IMO it is not worth removing more, because we are going off the rails 😃 🚂

dhh · 2017-07-27T19:29:09Z

The build is currently failing. Can you have a look?

dixpac · 2017-07-27T20:13:21Z

@dhh oh, sorry I fixed it :)

dixpac commented Jul 23, 2017

View reviewed changes

dixpac force-pushed the welcome_you_dummy branch from 872b3bf to e0cd338 Compare July 25, 2017 11:52

dixpac commented Jul 25, 2017

View reviewed changes

dhh reviewed Jul 26, 2017

View reviewed changes

dixpac force-pushed the welcome_you_dummy branch 4 times, most recently from b96f372 to 51c14ab Compare July 26, 2017 17:00

dixpac added 8 commits July 27, 2017 09:56

Introduce the Dummy.

85e1971

* Convert controller tests to integration tests * Move storage confogurations to dummy * Remove unecessary requires

Remove ActionCable

415395a

Remove ActionMailer

d178c8e

Remove sprockets and assets

346856f

Remove ActionView

0869ab1

Remove unecessary config files

98c208a

A lot smaller dummy app

1ca252e

Rename rails_disk_blob => rails_disk_service

93b1d03

dixpac force-pushed the welcome_you_dummy branch from 28b1d3d to fa20b41 Compare July 27, 2017 08:18

Bring back ActionView, Sprockets, Assets

d92b9f7

dixpac force-pushed the welcome_you_dummy branch from fa20b41 to d92b9f7 Compare July 27, 2017 20:08

dhh merged commit e64e3f1 into rails:master Jul 27, 2017

Introduce the Dummy. #70

Introduce the Dummy. #70

Uh oh!

Conversation

dixpac commented Jul 23, 2017

Uh oh!

dixpac Jul 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhh commented Jul 24, 2017

Uh oh!

dixpac Jul 25, 2017

Choose a reason for hiding this comment

Uh oh!

dhh Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

dixpac Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dixpac Jul 25, 2017

Choose a reason for hiding this comment

Uh oh!

dixpac commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dixpac commented Jul 26, 2017

Uh oh!

dhh Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

dhh commented Jul 26, 2017

Uh oh!

dhh commented Jul 26, 2017

Uh oh!

dixpac commented Jul 26, 2017

Uh oh!

kaspth commented Jul 26, 2017

Uh oh!

dixpac commented Jul 26, 2017

Uh oh!

dhh commented Jul 26, 2017 via email

Uh oh!

dixpac commented Jul 26, 2017 via email

Uh oh!

dhh commented Jul 26, 2017

Uh oh!

dixpac commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhh commented Jul 27, 2017

Uh oh!

dixpac commented Jul 27, 2017

Uh oh!

Uh oh!

dixpac Jul 23, 2017 •

edited

Loading

dixpac Jul 25, 2017 •

edited

Loading

dixpac commented Jul 25, 2017 •

edited

Loading

dixpac commented Jul 27, 2017 •

edited

Loading