Skip to content

feat(ui): require organization selection before entering pages#63

Open
Thorben-D wants to merge 1 commit into
mainfrom
force_taxonomy
Open

feat(ui): require organization selection before entering pages#63
Thorben-D wants to merge 1 commit into
mainfrom
force_taxonomy

Conversation

@Thorben-D
Copy link
Copy Markdown
Contributor

Before this commit, the "Content", "Execution Environment" and "Environments" pages would load incorrectly, if the user had selected "Any Organization".
Management of these objects across organizations is currently not supported.

To ensure the user has selected an organization, a new component, "ForceTaxonomy", is introduced, which requires the user to select an organization before proceeding.

The ForceTaxonomy component is generalized and supports enforcement of:

  • Organization selection
  • Loction selection
  • Organization and Location selection

Now, to access the "Content", "Execution Environments" and "Environments" pages, the user has to have a concrete organization selected.

Screenshots:

image

For illustration of the location requirement ability, not currently used:
image

Before this commit, the "Content", "Execution Environment" and
"Environments" pages would load incorrectly, if the user
had selected "Any Organization".
Management of these objects across organizations is currently
not supported.

To ensure the user has selected an organization, a new
component, "ForceTaxonomy", is introduced, which requires
the user to select an organization before proceeding.

The ForceTaxonomy component is generalized and supports
enforcement of:
- Organization selection
- Loction selection
- Organization and Location selection

Now, to access the "Content", "Execution Environments" and
"Environments" pages, the user has to have a concrete
organization selected.
@sbernhard
Copy link
Copy Markdown
Member

I would prefer to re-use something from foreman. Is there already somthing like this available? Or can you send a PR to foreman to add such a component (without Typescript as they are using React only)

@Thorben-D
Copy link
Copy Markdown
Contributor Author

I would prefer to re-use something from foreman. Is there already somthing like this available? Or can you send a PR to foreman to add such a component (without Typescript as they are using React only)

Unfortunately, such a thing does not currently exist in Foreman. The Katello implementation is written in Angular, and can't be reused.
I'm not sure there is a usecase for this in Foreman, given that it wasn't needed so far.

@sbernhard
Copy link
Copy Markdown
Member

Unfortunately, such a thing does not currently exist in Foreman. The Katello implementation is written in Angular, and can't be reused. I'm not sure there is a usecase for this in Foreman, given that it wasn't needed so far.

Can you reach out to Ian and/or Maria to clarify if there is a need for this? Would be easier for everyone to have one component which can be re-used in plugins - and then I think, its necessary to have it Foreman itself.

@Thorben-D
Copy link
Copy Markdown
Contributor Author

I can, but that is out of scope for this PR.
As long as Katello uses Angular, they won't need this, as this implementation can not be used with Angular.

@sbernhard
Copy link
Copy Markdown
Member

As long as Katello uses Angular, they won't need this, as this implementation can not be used with Angular.

Katello is currently moving forward with the transition to React.

See:
https://community.theforeman.org/t/ui-plan-transitioning-from-pf3-erb-and-angular-to-patternfly-6/45946/9
Katello/katello#11693 (and there is more)

Copy link
Copy Markdown
Contributor

@nadjaheitmann nadjaheitmann left a comment

Choose a reason for hiding this comment

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

I could not test the location part but the organization part works as expected.

addToast({
type: 'success',
key: `UPDATE_CURRENT_${type.toUpperCase()}_SUCC`,
message: `Successfully updated the current ${type}!`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The message needs a translation.

addToast({
type: 'danger',
key: `UPDATE_CURRENT_${type.toUpperCase()}_ERR`,
message: `Updating the current ${type} failed with error code "${
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The message needs a translation.

@maximiliankolb
Copy link
Copy Markdown
Contributor

Looks like there's already some code duplication on HEAD of "main" too:

$ rg addToast | wc
     60     184    7404

$ rg -A 3 addToast
webpack/components/ansible_execution_environments/components/ExecutionEnvGridWrapper.tsx
13:import { addToast } from 'foremanReact/components/ToastsList';
14-import { translate as _ } from 'foremanReact/common/I18n';
15-
16-import { useDispatch } from 'react-redux';
--
99:          addToast({
100-            type: 'success',
101-            key: `DESTROY_EE_${selectedEnv.name}_SUCC`,
102-            message: `Successfully deleted Ansible Execution Environment "${selectedEnv.name}"!`,
--
109:          addToast({
110-            type: 'danger',
111-            key: `DESTROY_EE_${selectedEnv.name}_ERR`,
112-            message: `Deletion of Ansible Execution Environment "${
--
141:          addToast({
142-            type: 'success',
143-            key: `UPDATE_EE_${selectedEnv.name}_SUCC`,
144-            message: `Successfully updated Ansible Execution Environment "${selectedEnv.name}"!`,
--
150:          addToast({
151-            type: 'danger',
152-            key: `UPDATE_EE_${selectedEnv.name}_ERR`,
153-            message: `Updating of Ansible Execution Environment "${
--
181:        addToast({
182-          type: 'success',
183-          key: `CREATE_EE_${env.name}_SUCC`,
184-          message: `Successfully created Ansible Execution Environment "${env.name}"!`,
--
191:        addToast({
192-          type: 'danger',
193-          key: `CREATE_EE_${env.name}_ERR`,
194-          message: `Creation of Ansible Execution Environment "${
...

<CardTitle>
<TextContent>
<Text component={TextVariants.h2}>
{_('An organization is required to proceed.')}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
{_('An organization is required to proceed.')}
{_('Select an organization.')}

Please check if all of those texts end with a dot or not.

Same suggestion applies to line 173 and 131.

@nadjaheitmann
Copy link
Copy Markdown
Contributor

Looks like there's already some code duplication on HEAD of "main" too:

$ rg addToast | wc
     60     184    7404

$ rg -A 3 addToast
webpack/components/ansible_execution_environments/components/ExecutionEnvGridWrapper.tsx
13:import { addToast } from 'foremanReact/components/ToastsList';
14-import { translate as _ } from 'foremanReact/common/I18n';
15-
16-import { useDispatch } from 'react-redux';
--
99:          addToast({
100-            type: 'success',
101-            key: `DESTROY_EE_${selectedEnv.name}_SUCC`,
102-            message: `Successfully deleted Ansible Execution Environment "${selectedEnv.name}"!`,
--
109:          addToast({
110-            type: 'danger',
111-            key: `DESTROY_EE_${selectedEnv.name}_ERR`,
112-            message: `Deletion of Ansible Execution Environment "${
--
141:          addToast({
142-            type: 'success',
143-            key: `UPDATE_EE_${selectedEnv.name}_SUCC`,
144-            message: `Successfully updated Ansible Execution Environment "${selectedEnv.name}"!`,
--
150:          addToast({
151-            type: 'danger',
152-            key: `UPDATE_EE_${selectedEnv.name}_ERR`,
153-            message: `Updating of Ansible Execution Environment "${
--
181:        addToast({
182-          type: 'success',
183-          key: `CREATE_EE_${env.name}_SUCC`,
184-          message: `Successfully created Ansible Execution Environment "${env.name}"!`,
--
191:        addToast({
192-          type: 'danger',
193-          key: `CREATE_EE_${env.name}_ERR`,
194-          message: `Creation of Ansible Execution Environment "${
...

I don't see how you could improve that code much TBH.

@nadjaheitmann
Copy link
Copy Markdown
Contributor

What is holding us back here except the strings?

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.

4 participants