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

[WIP] user-management react module #4562

Closed
wants to merge 26 commits into from

Conversation

Hyperkid123
Copy link
Contributor

@Hyperkid123 Hyperkid123 commented Aug 29, 2018

This should not be included to Hammer release.

This PR replaces the whole user section in access control with React components.

Changes

  • State management via redux
  • Client routing using react-router (also connected to tree-view-controller and toolbars)
  • Using miq API where possible
  • Data lazy loading (no server roundtrip)

TO-DO

EDIT: most of the PR are test related files i did not write 3k+ lines of code

@miq-bot miq-bot added the wip label Aug 29, 2018
@Hyperkid123
Copy link
Contributor Author

Hyperkid123 commented Aug 29, 2018

@miq-bot add_reviewer @karelhala
@miq-bot add_reviewer @martinpovolny
@miq-bot add_reviewer @himdel
@miq-bot add_reviewer @dclarizio
@miq-bot add_reviewer @apagac

@Hyperkid123
Copy link
Contributor Author

@miq-bot add_label react

@miq-bot
Copy link
Member

miq-bot commented Aug 29, 2018

@Hyperkid123 'apagac' is an invalid reviewer, ignoring...


listenToRx(function(payload) {
if (payload.type === 'init-react-routing' && !vm.reactRouting) {
vm.reactRouting = !!payload.reactRouting;
vm.$scope.$apply;
Copy link
Contributor

Choose a reason for hiding this comment

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

missing ()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch!

@Hyperkid123
Copy link
Contributor Author

@ZitaNemeckova @lpichler


def get_category_entries
x = Classification.find_by_id(params[:cat_id])
render :json => x.entries.map { |entry| { :value => entry[:id], :label => entry[:description], :name => entry.tag[:name] } }
Copy link
Contributor

Choose a reason for hiding this comment

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

entries.includes(:tag) to avoid N+1 queires and you can use . notation { :value => entry.id,...` to access attributes.

thanks

def get_category_entries_multi
category_entries = {}
params[:ids].map { |category_id|
cat = Classification.find_by_id(category_id)
Copy link
Contributor

@lpichler lpichler Aug 31, 2018

Choose a reason for hiding this comment

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

I believe that this could be done in query
Classification.where(:ids => params[:ids]).includes(:tags)....

I see similar pattern in such queries so maybe you will find how to unify it to one method and put it on model.

@Hyperkid123 Hyperkid123 force-pushed the rbac-react-module branch 11 times, most recently from e593edf to ac464bf Compare September 5, 2018 12:11
@Hyperkid123
Copy link
Contributor Author

@dclarizio @martinpovolny @lpichler i think it is ready for some clicking in the UI. You just need to add this PR ManageIQ/manageiq-api#464 to your API plugin.

@miq-bot
Copy link
Member

miq-bot commented Nov 12, 2018

Checked commits Hyperkid123/manageiq-ui-classic@ad51220~...27ee4e9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
14 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

@Hyperkid123
Copy link
Contributor Author

@dclarizio @himdel ok after a lot debugging hacking and discussing, we (me and @himdel) have decided that introducing client-side routing at this stage will create more problems than we have right now. Until other major changes are done to the app, adding client side routing to existing parts of the ui is not very beneficial. There is a ton of interactions between trees, toolbars and other that make this unnecessarily complicated (the code shows it) and not very maintainable.

I will extract all the components i have made simply put them inside the layout without the routing.

@Hyperkid123 Hyperkid123 deleted the rbac-react-module branch November 12, 2019 09:13
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.

5 participants