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 #24480 - fix org switching on subs page #7605

Merged
merged 1 commit into from Sep 18, 2018

Conversation

amirfefer
Copy link
Contributor

@amirfefer amirfefer commented Aug 7, 2018

Certain flows will allow the user to see subs across any organization.
This PR fixes it - when clicking on the Any Organization , a select organization page should appear.

@theforeman-bot
Copy link

Issues: #24480

Copy link
Contributor

@waldenraines waldenraines left a comment

Choose a reason for hiding this comment

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

Works great, thanks @amirfefer!

Can you add a test for the new functionality please?

}
static getDerivedStateFromProps(newProps) {
const uiOrgName = document.getElementById('organization-dropdown').children[0].text.trim();
if (uiOrgName === 'Any Organization' && !newProps.location.state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little fragile, can we just check for the lack of an org id?

@amirfefer
Copy link
Contributor Author

@waldenraines I've added some tests

@waldenraines
Copy link
Contributor

@amirfefer this works well and thanks for adding tests but this now needs a rebase.

path: 'redhat_repositories',
component: WithOrganization(Repos, '/redhat_repositories'),
component: WithOrganization(
withHeader(Repos, { title: __('RH Repos') }),
Copy link
Contributor Author

@amirfefer amirfefer Sep 12, 2018

Choose a reason for hiding this comment

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

The HOC's order is importent - affects on the entire component's lifecycle


export default () => (
<div>
{links.map(({ path, component, text }) => {
const Page = component;
const withHeader = () => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

on every route change withHeader will recreate, it should exist as a separate component.

@amirfefer
Copy link
Contributor Author

@waldenraines I've rebased and fix some issues, would you mind to have a quick look?

package.json Outdated
"enzyme": "^3.2.0",
"enzyme-adapter-react-16": "^1.1.0",
"enzyme": "^3.4.0",
"enzyme-adapter-react-16.3": "^1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes required for this? If so we need to add a request to update these packages downstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

works locally without these.

@waldenraines
Copy link
Contributor

@amirfefer sorry, I created some more conflicts for you here by merging #7630

@theforeman-bot
Copy link

There were the following issues with the commit message:

  • df7d2a3 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@waldenraines
Copy link
Contributor

waldenraines commented Sep 14, 2018

There were the following issues with the commit message:

Dang, I was hoping I could fix these conflicts for you and merge but apparently not. At any rate they are very minor.

@amirfefer
Copy link
Contributor Author

@waldenraines - rebased :)

@waldenraines
Copy link
Contributor

[test katello]

@waldenraines waldenraines merged commit 0d98723 into Katello:master Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants