Update layout_for_admin component to take custom asset filenames

[davidgisbey]

What

Allow custom JS and CSS filenames to be passed to the layout_for_admin component.

Why

We're adding an admin interface to our application and want to use retain application.js & application.scss for the public interface.

This will allow us to pass admin.js and admin.scss in as args so we can retain only loading the required components for each so the page load time is fast.

Visual Changes

N/A

Testing

It was a bit awkward to test this as passing it a filename that doesn't exist causes exceptions. Using any_instance_of is a bit of a smell, but it's probs okay(ish) here to avoid additional complexity.

Trello card

https://trello.com/c/r6uafy2D/1354-basic-admin-area

[kevindew]

Great stuff David 👍

[kevindew]

Great, all the changes look good to me. Only thing I remembered to suggest is that we should add some information in the commit message about how we ended up committing the sin of using (allow|expect)_any_instance_of and how we ended up stubbing ActiveView::Base - as there is quite opaque.

It'd be good to get a nod from @andysellick or one of the frontend devs on our team before merging though

[davidgisbey]

Yea great shout. Sorry i was using CI to run my tests as i was having issues locally and forgot to update the commit message. I'll sort that now 👍

[andysellick]

Change looks good, just a few comments/questions 👍

[andysellick]

These examples pass the default value, would it be clearer to show this functionality in action?

[davidgisbey]

The problem with that is that i'd have to add a blank admin stylesheet, because it will actually try to compile the file. So it if update it to use "admin" it'll look for "admin.js" and blow up if it can't find it.

I figured this was the lesser of 2 evils, but let me know if you'd prefer another approach 👍

[andysellick]

Oh I see that makes sense. Tricky! In that case maybe add a description to these examples to clarify? Something like:

description: An alternative JS filename can be used in place of the default `application.js` if required (note that this cannot easily be demonstrated here).

[davidgisbey]

great shout. Thanks Andy will sort that now

[andysellick]

Sorry, description needs to be added at the top level, underneath the example name, e.g.

with_custom_js_filename:
  description: etc.
  data:
    environment: production

It should appear in the component guide if you want to check it.

[andysellick]

Again just interested to know how this works - why does the JS test have allow_any_instance_of and expect_any_instance_of, but the CSS test only has expect_any_instance_of?

[davidgisbey]

Similar to above, the reason we're unable to make an assertion on the page is that it will blow up while trying to render the component if it's unable to find the file based on filename passed into the component.

Due to that we have to stub out the method that handles the compilation of the assets. It feels like in this situation the thing we really care about is that the correct file is being included.

How allow and expect is that they stub out the method for the class passed in and return "" (by default).
So essentially what happens here is that the layout makes the call to compile admin.js and we intercept that call and return an empty string. Blocking it from actually trying to compile and blowing up.

The reason that we need allow_any_instance_of in the JS test is due to there being a second method call here

So two calls are made:

 <%= javascript_include_tag "govuk_publishing_components/vendor/modernizr" %>

then

 <%= javascript_include_tag "application" %>

Without the allow this line

 expect_any_instance_of(ActionView::Base).to receive(:javascript_include_tag).with("admin").once

blows up because the javascript_include_tag method has been stubbed, but only once with expectation that the argument will be "admin".

The output of the test is something similar to

expected  javascript_include_tag with the argument of "admin", but it was actually "govuk_publishing_components/vendor/modernizr"

The allow allows the method to be called with any argument any amount of times which ensures we don't see the error above. While the expect is checking that the call is made with "admin" once which is the desired out come

Hopefully that makes sense.

Maybe it's clearer if i update the allow to be more explicit and do

allow_any_instance_of(ActionView::Base).to receive(:javascript_include_tag).with("govuk_publishing_components/vendor/modernizr")
 expect_any_instance_of(ActionView::Base).to receive(:javascript_include_tag).with("admin").once

So it's clear that the allow is due to the method being called twice, once with a file we're not worried about with our change to the implementation.

[davidgisbey]

There are other approaches we could use like dynamically creating an empty admin.js file before action before running the test, testing what's in the file, then deleting it after running the test, but that comes with it's own setbacks. Still complex, far slower etc.

But happy to chat it through if you'd like!

I'll go back and update the commit message with some of the above.

[andysellick]

Really interesting stuff, thanks for taking the time to explain 👍

[davidgisbey]

@andysellick i've made the requested changes. Hopefully the above explanation makes sense. Let me know if you not and/or you want any other changes 👍

[davidgisbey]

I've tested it's working as expected on the component guide so i'll merge if there's no objections