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 #37228 - Replace Ansible/Roles page with React component #696

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Thorben-D
Copy link
Contributor

@Thorben-D Thorben-D commented Mar 6, 2024

Feature #37228

The new component acts as a drop-in replacement for the previously used erb-table.

This is required by #707

centos8-katello-devel-stable example com_ansible_ansible_roles_order=name+asc

@sbernhard
Copy link
Contributor

What is blocking us from using this change? Possibly at the start of a new release cycle?

@nofaralfasi
Copy link
Contributor

What is blocking us from using this change? Possibly at the start of a new release cycle?

I'll review it ASAP.

@nofaralfasi
Copy link
Contributor

The functionality works well. However, the page is loading much slower than before. I’m wondering if it’s an issue on my side or if the new code is causing the app to load slower.

@Thorben-D
Copy link
Contributor Author

@nofaralfasi
Unfortunately, this is a side-effect of React rendering the component on the client-side. I am unsure when exactly react-rails renders components, but it feels pretty late (c.f. /new/hosts). Rendering server-side would help in this regard, but that does not work when window or document are used.

@sbernhard
Copy link
Contributor

Of course, the site load would be pretty fast but rendering the page takes longer as its done in the browser itself.
Should it have a spinner as this was done in Katello/Content-views for example?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

In Foreman we have the TableIndexPage component which is intended for these index pages. Have you considered using that and if so, why was it insufficient?

Copy link
Member

@MariaAga MariaAga left a comment

Choose a reason for hiding this comment

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

+1 to Ewouds question, the js loading is slower between ruby and js, but faster between full React pages.
Can this be a full react page? it will need to be added in webpack/routes/routes.js, config/routes.rb

app/views/ansible_roles/index.html.erb Outdated Show resolved Hide resolved
@Thorben-D
Copy link
Contributor Author

Thorben-D commented May 17, 2024

Sorry for the delay, I was occupied otherwise.
Thank you for your feedback. I would actually prefer a full React page as-well! I chose this approach to keep the changes as low-profile as possible, but I am happy to move it to a full page.
Regarding TableIndexPage, I'm afraid, I couldn't get it to work... Do you have an example where this was used outside of foreman? (The page not the hooks!)

@Thorben-D
Copy link
Contributor Author

Thanks, that's what I was looking for.
I checked out the component and the biggest issue I see with it is the way the action buttons are handled.
The old page allows the selection of a smart proxy to import roles from through a dropdown button.
While ActionButtons does provide a dropdown, it is toggled by a "kebab" toggle and not a component with text on it.

@Thorben-D
Copy link
Contributor Author

^^Nevermind, I just noticed the customToolbarItems prop...

@Thorben-D
Copy link
Contributor Author

Sorry for the holdup, I was occupied otherwise.
I re-wrote the whole thing to use the TableIndexPage component. I decided to adapt the ui_ansible_roles endpoint for this, as I did not find any usages.
Tests are still missing, but I will of course add some once we agreed on the UI.

app/views/ui_ansible_roles/show.json.rabl Outdated Show resolved Hide resolved
app/views/ui_ansible_roles/show.json.rabl Outdated Show resolved Hide resolved
@@ -7,6 +10,12 @@ def index
@ui_ansible_roles = resource_scope_for_index(:permission => :view_ansible_roles)
end

api :DELETE, '/ui_ansible_roles/:id', N_('Deletes Ansible role')
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any permission filtering here. Am I missing something?

Also, couldn't this just use the regular REST API to delete roles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I forgot to add the permission.

The table component uses the same endpoint for querying and deleting items, so it's unfortunately not possible to use the API just for deletion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure this is the right approach? Adding an API call here doesn't seem right to me. I know it might be simpler, but we can still use the existing API call to delete a role. It might require some frontend adjustments, but it will align with the rest of our code.
For reference, this is how we do it on the new hosts page: https://github.com/theforeman/foreman/blame/6aac916fb183f47596e8d543756faac27750a21e/webpack/assets/javascripts/react_app/components/HostsIndex/index.js#L283

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this would require significant changes to TableIndexPage.js and Table.js in core, as the idea is that TableIndexPage represents an interface to a single controller that has the necessary CRUD methods.
One could add another prop, deletionURL to TableIndexPage and Table that would take precedence over the original URL for deletion but I am not sure that is a cleaner solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we add the Delete option to the rowKebabItems action items instead of using the isDeleteable property? From there, we can use the URL of the existing API for the delete action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, we would lose the delete modal that way.

@Thorben-D Thorben-D force-pushed the feature/37228_ansible_roles_page_react branch 2 times, most recently from 57587ef to d2b6c5c Compare June 21, 2024 13:22
@Thorben-D
Copy link
Contributor Author

I added some tests for the React components.
I decided to omit a test for the AnsibleRolesPage component, as it is basically just a static wrapper for TableIndexPage, which is already extensively tested.

Copy link
Contributor

@nofaralfasi nofaralfasi left a comment

Choose a reason for hiding this comment

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

The page is significantly improved now. Thank you, @Thorben-D, for your hard work!

Here are a few notes from me:

  • The "Import from" button is positioned on the same row as the top navigation, causing it to appear centered on the page.
  • Table rows have a clickable cursor, but clicking them does not trigger any action.
  • Clicking the Documentation button opens it in the current window.
  • If the current user lacks permissions to view hosts or hostgroups, the corresponding options in the action menu should be disabled (same as we have now for the delete option).
  • When the current user has permissions only to import roles (without viewing permissions), loading the Ansible Roles page results in the following error message: "There was an error requesting Smart Proxies," and the smart-proxy cannot be selected.

@MariaAga
Copy link
Member

The "Import from" button is positioned on the same row as the top navigation, causing it to appear centered on the page.

This is fixed in foreman core

Table rows have a clickable cursor, but clicking them does not trigger any action.
Clicking the Documentation button opens it in the current window

Those are issues from foreman core

@Thorben-D
Copy link
Contributor Author

Thanks for testing! I looked into the error you discovered.
It was caused by the user not having the view_smart_proxies permission. I added a check for that one as well and now the dropdown works correctly.
Apparently, the old UI also implicitly required this permission. It does make sense if you think about it, but I feel like permissions that depend on other permissions should be avoided... Not sure.

Regarding the "view X" buttons, while it is of course possible to hide those based on permissions, I would either have to lift the existing request up or add a second one to make it work. Both solutions would noticeably slow the page down however.
At that opportunity, I want to shamelessly refer you to this proposal, which would solve this issue cleanly.
Also, now If a user tries to view hostgroups for example, he is informed that he would need the view_hostgroups permission to do so. I feel like that could have some value. Not sure what would be best here either...

@sbernhard
Copy link
Contributor

@nofaralfasi / @ekohl This PR is now open since Mar 6 and we invested a lot to improve foreman_ansible here. @Thorben-D answered all the questions and worked hard to get it done.
What is preventing us from merging this?

@nofaralfasi
Copy link
Contributor

nofaralfasi commented Aug 1, 2024

@Thorben-D Could you please rebase the PR? I'll re-check the permission issue, and let's see if the tests pass after the rebase.

Update:
Deleting Ansible Roles option isn't enabled on the Roles page, for a non-admin user with destroy_ansible_roles permission.

@Thorben-D Thorben-D force-pushed the feature/37228_ansible_roles_page_react branch 4 times, most recently from 9e9cd95 to 17b614a Compare August 2, 2024 10:46
@Thorben-D
Copy link
Contributor Author

Thank you for having another look!
Indeed, there was an issue with permissions again. Apparently, the authorized_for method I used before did not behave correctly. I replaced it with a different solution.
Test failure is also fixed.
Sorry about the linting, I am getting some psych error once again on my test-setup. It's nice though that linting issues are now shown in the diff!

:params => { :id => @role.id },
:session => set_session_user
assert_response :success
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test to verify that a user without the destroy_ansible_roles permission cannot perform this action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if this is necessary. The permissions should already be tested by a test in core and I didn't really find an existing test that tested this.

Copy link
Contributor

@sbernhard sbernhard Aug 9, 2024

Choose a reason for hiding this comment

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

@MariaAga what are your thoughts about this?

Copy link
Member

Choose a reason for hiding this comment

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

Not my area, but I couldnt find a test in core for it in core

@nofaralfasi
Copy link
Contributor

Overall, it looks good to me. Once Ewoud's and my comments are addressed, I believe we should merge this

@Thorben-D Thorben-D force-pushed the feature/37228_ansible_roles_page_react branch from 17b614a to 28c9905 Compare August 6, 2024 11:08
@nofaralfasi
Copy link
Contributor

I would expect to receive a proper error message when the user doesn't have the necessary permissions to view this page.
image

@Thorben-D
Copy link
Contributor Author

@nofaralfasi The server actually sends back the following message:
"Missing one of the required permissions: view_ansible_roles"
Unfortunately though, the useAPI hook, currently, does not store this information in the store.
Given that a user does not normally get to this stage via the navigation, I think this is acceptable for now.

@nofaralfasi
Copy link
Contributor

nofaralfasi commented Aug 13, 2024

@Thorben-D what about permissions to view hosts or hostgroups? I think we should disable the buttons that are not permitted in the action menu.

Update: after discussing it with @ekohl, we agreed that to maintain consistency across the UI, this table should be aligned with the others. Specifically, the view actions for hosts and hostgroups should be presented as links within their respective column counts.

@Thorben-D Thorben-D force-pushed the feature/37228_ansible_roles_page_react branch from 28c9905 to ca5bdfb Compare August 28, 2024 10:17
@Thorben-D
Copy link
Contributor Author

Apologies, I did not see the last comment. I updated the design to match the desired outcome. Links in the columns will only be given if a user has the authorization to view them.

@Thorben-D Thorben-D force-pushed the feature/37228_ansible_roles_page_react branch from ca5bdfb to 5f218bc Compare August 28, 2024 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants