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

Fixes #30350 - adds the scoped_search form parameter #8824

Merged

Conversation

jjeffers
Copy link
Member

@jjeffers jjeffers commented Jul 8, 2020

This PR repairs bulk host actions for errata (and packages) modals from the content hosts summary page.

The "scoped_search" form parameter was missing and here we attempt to populate that value before the form is submitted.

To test this change, register at least as many content hosts as the page volume allows, plus 1 more. (You want more hosts to be selectable than can be shown in 1 page of results.)

Screenshot from 2020-07-10 13-51-13

Next, select top checkbox for all hosts, then click the "Select All ..." link near the top to the results table. Then choose an action from the Select Action option, picking "Manage Errata".

Assuming there is applicable errata indicated for at least one of the hosts, select that errata. Then click "Install" with the "remote execution" option. The subsequent task to install the errata should be applied against all hosts (not just the ones shown on the first page).

Note that the same errors appear for "Manage Packages", and this PR repairs that functionality as well.

@theforeman-bot
Copy link

Issues: #30350

@jturel
Copy link
Member

jturel commented Jul 10, 2020

This change does fix the problem but it took me a little while to figure out why you had to go the route you did, and I learned some things:

  • the hostIds data is derived from the getAllSelectedResults function in Nutupane (the container object of the content hosts table)

self.getAllSelectedResults = function (identifier) {
var selected, selectedRows;
identifier = identifier || 'id';
selected = {
included: {
ids: [],
resources: [],
search: null,
params: params
},
excluded: {
ids: []
}
};
if (self.table.allResultsSelected) {
selected.included.search = self.table.searchTerm || '';
selected.excluded.ids = _.map(self.getDeselected(), identifier);
} else {
selectedRows = self.table.getSelected();
selected.included.ids = _.map(selectedRows, identifier);
selected.included.resources = selectedRows;
}
return selected;
};

  • that function creates a data structure that we basically send as-is to the API
  • i don't like how tightly coupled some random function in our JS code is unintentionally coupled to our API

OK, rant over :)

The hostIds doesn't give you an easy way to ask "did we have all hosts selected?" and that's what you're trying to determine by checking the length of the included ids. So, I'm wondering if we can tag on an extra attribute in the object returned by getAllSelectedResults which can allow us to answer that question more easily. It should help clean up the logic a bit, and make the intention clearer. Unfortunately you'll need to do a bit of a read in the code to see if we're going to end up sending that new value to the API or something. If not, then I think it's a quick and worthwhile change. Let me know what you think

@jjeffers
Copy link
Member Author

@jturel thank you for the feedback. This is precisely the sort of review I like to get.

I think the request to make the state of the selection more apparent is a good one. Let me see what we can come up with.

@jjeffers
Copy link
Member Author

@jturel I don't think the additional attribute is passed directly to the API. In both controllers the returned data from nutupane getAllSelectedResults is then injected into form values, and not directly supplied.

I'm not sure what the other controllers do, so I'm looking into that now.

@@ -38,6 +38,7 @@ <h4 data-block="modal-header" translate>Update Packages</h4>
<input type="hidden" name="host_ids" ng-value="packageActionFormValues.hostIds"/>
<input type="hidden" name="authenticity_token" ng-value="packageActionFormValues.authenticityToken"/>
<input type="hidden" name="customize" ng-value="packageActionFormValues.customize"/>
<input type="hidden" ng-if="scopedSearch" name="scoped_search" ng-value="errataActionFormValues.scopedSearch"/>
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be packageActionFormValues.scopedSearch

$scope.packageActionFormValues.hostIds = selectedHosts.included.ids.join(',');
}

if (selectedHosts.allResultsSelected) {
$scope.scopedSearch = true;
Copy link
Member

Choose a reason for hiding this comment

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

what about this?

$scope.allHostsSelected = selectedHosts.allResultsSelected;

if ($scope.allHostsSelected) {
...
}

That way you don't need to track scopedSearch separately and you can use it in the view as part of your conditional logic

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea!

if (selectedHosts.included.ids) {
if ($scope.allHostsSelected) {
$scope.packageActionFormValues.scopedSearch = selectedHosts.included.search;
} elseif (selectedHosts.included.ids.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess it should be 'else if' 🙄

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah the linter smacked my hand

Copy link
Member

@jturel jturel left a comment

Choose a reason for hiding this comment

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

Works perfectly - APJ

@jturel jturel self-assigned this Jul 14, 2020
@jjeffers jjeffers force-pushed the 30350_bulk_actions_with_select_all branch from c84230d to 81ef393 Compare July 14, 2020 15:19
@jjeffers jjeffers merged commit 11f0ce3 into Katello:master Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants