fixes #12419 - template preview host has typeahead

[timogoebel]

No description provided.

[timogoebel]

[test foreman]

[ares]

I'm afraid this change will break template previewing for Job templates used in remote execution plugin. Note that there we also want to be able to preview templates for unmanaged hosts.

[timogoebel]

Thanks for the hint, it was definitely not the aim of this PR to break remote execution ;-)

[ares]

Sure :-) yesterday I tested this PR and it works very nicely! I don't feel for the full review now but from quick scan this was the only thing that grabbed my attention.

[ohadlevy]

@timogoebel I don't think anyone wants to mess out with Remote Execution... you could never know when your schedule is next....

[timogoebel]

Pushed a plugin friendly version.

[tbrisker]

In general works well, a few inline comments on the js code.
Also didn't test this on any plugins - @ares can you verify this works correctly with REX? I'm not sure what you leftover comment was about and if it was indeed fixed.

[tbrisker]

Mind turning this into a generic auto-completer function that gets an input as a parameter? and then integrate it into foreman_tools?

[tbrisker]

Also, please add a small test to it.

[tbrisker]

I think you can use input.data('url')

[tbrisker]

ditto

[tbrisker]

you can use some fun ES6 here, something like:

results: (data) => ({results: data.map( ({id, name}) => ({id, text: name}) )});

[tbrisker]

might want to leave the styling to css so it can be easily changed.

[tbrisker]

you can make this (and others) into an arrow function.

[tbrisker]

what does this do?

[ares]

Also didn't test this on any plugins - @ares can you verify this works correctly with REX? I'm not sure what you leftover comment was about and if it was indeed fixed.

I just tested with REX and works like a charm 👍

[houndci-bot]

This function has too many statements (25). Maximum allowed is 10 max-statements

[timogoebel]

@tbrisker : Addressed your comments and added a (quite complex) test.

[tbrisker]

just a few more comments inline

[tbrisker]

I think this should move into the if show_preview block below, no point in running this if the field doesn't exist.

[tbrisker]

I'm not sure I completely understand this test, but it looks like you are verifying the internals of select2 work correctly? Would it be possible to just mock $.ajax so it returns results in the correct format when called, then check that the field has indeed been initialized correctly after the call (i.e. the correct options are there)? That way if something in the internals changes we wouldn't care unless it broke the functionality.

[timogoebel]

@tbrisker : I believe, it's quite hard to actually test that select2 generates a select field. Imho this would actually be testing that select2 works. The test I wrote just checks, that the correct data is passed to select2 and that our callbacks work correctly.

[tbrisker]

I guess what I would want to test here is that there is a field that has this initialized and check that select2 has the correct response, without mocking out everything inside the function itself. So I guess just mocking the ajax calls to get the correct responses, and making sure the field is populated correctly.

[tbrisker]

I think it might make sense to keep the default limit in case a plugin using this function doesn't override it so that not all hosts will be loaded, what do you say?

[timogoebel]

I think, this should be handled by the plugin itself. If the plugin needs a limit, it can easily use preview_host_collection.limit(100).

[ares]

I don't think plugins need to override this (at least not REX), the previewing should work the same for each template type. I hope I'm not missing something.

[timogoebel]

@tbrisker : Thanks for the review. Moved the js code into the if condition. Regarding the tests: I'm not 100% sure, what kind of test you are suggesting. Maybe I should refactor "my" test to make it more readable?

[tbrisker]

Works well other then the test, which I feel does a bit too much mocking and checking the internals of the function/select2, rather then just checking that running the function on an input field produces the expected results.

[tbrisker]

I guess what I would want to test here is that there is a field that has this initialized and check that select2 has the correct response, without mocking out everything inside the function itself. So I guess just mocking the ajax calls to get the correct responses, and making sure the field is populated correctly.

[houndci-bot]

Expected blank line after variable declarations newline-after-var

[timogoebel]

diff --git a/webpack/assets/javascripts/foreman_tools.test.js b/webpack/assets/javascripts/foreman_tools.test.js
index 62380b9..1abd191 100644
--- a/webpack/assets/javascripts/foreman_tools.test.js
+++ b/webpack/assets/javascripts/foreman_tools.test.js
@@ -60,48 +60,17 @@ describe('deprecate', () => {

 /* eslint-disable max-statements */
 describe('initTypeAheadSelect', () => {
-  it('calls select2 on given input field', () => {
+  it('initializes select2 on given input field', () => {
     const $ = require('jquery');
+    require('select2');

     document.body.innerHTML =
       '<input type="text" id="typeahead" data-url="testurl" data-scope="testscope">';

     let field = $('#typeahead');

-    field.select2 = jest.fn();
     tools.initTypeAheadSelect(field);
-    expect(field.select2).toHaveBeenCalledTimes(1);

-    let arg = field.select2.mock.calls[0][0];
-
-    expect(arg.ajax.url).toBe('testurl');
-
-    let data = arg.ajax.data('testterm', 'testpage');
-
-    expect(data.q).toBe('testterm');
-    expect(data.scope).toBe('testscope');
-
-    let testResults = [{id: 1, name: 'test'}];
-    let testResponse = {'id': 1, 'text': 'test'};
-    let results = arg.ajax.results(testResults).results;
-
-    expect(results.length).toBe(1);
-    expect(results[0]).toEqual(testResponse);
-
-    let doneMock = jest.fn();
-    let callbackMock = jest.fn();
-
-    $.ajax = jest.fn();
-    $.ajax.mockReturnValue({done: doneMock});
-
-    arg.initSelection('', callbackMock);
-
-    expect($.ajax).toBeCalledWith('testurl', {data: {scope: 'testscope'}, dataType: 'json'});
-
-    let callback = doneMock.mock.calls[0][0];
-
-    callback(testResults);
-
-    expect(callbackMock).toBeCalledWith(testResponse);
+    expect(document.body.innerHTML).toContain('select2-container');
   });
 });

@tbrisker : Thanks. I struggled to mock the ajax call for some time only to realize there is no way to check the call or the data using the html code. I pushed another suggestion. Need some help here, it this is not what you had in mind...

[tbrisker]

@timogoebel I tried doing it myself and I now understand the difficulty of mocking ajax calls... @matanwerbner maybe you have an idea? I don't want to test all of the internals of the function but rather to make sure the input gets initialized correctly.

[matanwerbner]

i would look into nock:
https://github.com/node-nock/nock
set it up, it should intercept any http requests to the host specified (can be done with regex)

[tbrisker]

@matanwerbner I took a look at nock and it seems like overkill here. we don't actually care about the ajax calls going out.
I've had some progress by mocking $.ajax to return a jQuery Deferred object, but still haven't figured out how to get the options to populate correctly - take a look at tbrisker@c3d0eae

[tbrisker]

A bit more progress - got the dropdown opened with "searching" placeholder in the list - tbrisker@3e5852c

[matanwerbner]

@tbrisker i would give testing http request another think..
since it is a big part of the component, firing both on init and on type, you might want to test that requests are firing correctly, including the query being sent.

[tbrisker]

@matanwerbner The issue here isn't that the mocking of the request doesn't work, rather that triggering the request from the test doesn't work. I don't want to introduce extra dependencies (and nock pulls in 14 other packages) if we can avoid it just to test something that can be easily mocked with the current setup.

[matanwerbner]

Ok you dont have to use nock, i forgive you 😏

[timogoebel]

@tbrisker : Anything I can do to get this done in time for 1.15?

[timogoebel]

@tbrisker : Rebased, added your tested and cleaned things up.

[tbrisker]

👍 pending all 🍏

[tbrisker]

Actually found one minor issue - if you switch the open template by clicking on another one, an error is printed to the console:

select2.js:2472 Uncaught TypeError: Cannot read property 'find' of null
    at constructor.updateSelection (select2.js:2472)
    at select2.js:2317
    at Object.<anonymous> (foreman_tools.js:76)
    at fire (jquery.js:3187)
    at Object.fireWith [as resolveWith] (jquery.js:3317)
    at done (jquery.js:8757)
    at XMLHttpRequest.<anonymous> (jquery.js:9123)

It still seems to work correctly though so I'm not sure what the issue is.

[timogoebel]

@tbrisker : I give up on this one. I suspect, it's a bug in select2 which might have been fixed in a later version. However I can't upgrade because the newer versions aren't on npm. If you have any idea what's going on, I'm happy to hear that.

[tbrisker]

@timogoebel thanks for trying, as it seems to be working just fine i'll merge this, hopefully upgrading to select2 v.4 will resolve it.

[tbrisker]

Thanks @timogoebel !