-
Notifications
You must be signed in to change notification settings - Fork 21
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
[AN-149] Rename 'namespace' to 'collection' & update feature preview section #5212
Conversation
src/libs/feature-previews-config.ts
Outdated
title: 'Terra Workflow Repository Improvements', | ||
description: | ||
"Enabling this feature will gives users access to the new Terra Workflow Repository UI. This replaces the Broad Methods Repository UI (and doesn't affect any workflows previously created).", | ||
feedbackUrl: `mailto:[email protected]?subject=${encodeURIComponent( | ||
'Feedback on Terra Workflow Repository Improvements' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reviewers: This was requested by Beth and I did it as part of this PR itself. Please let me know if there is any wording improvements that can be done, especially for the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
src/libs/feature-previews-config.ts
Outdated
'Feedback on deprecating Firecloud UI' | ||
title: 'Terra Workflow Repository Improvements', | ||
description: | ||
"Enabling this feature will gives users access to the new Terra Workflow Repository UI. This replaces the Broad Methods Repository UI (and doesn't affect any workflows previously created).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I attempted to emphasize what had been missing in Terra (create & edit) and try to explain that these are two UIs viewing the same backend data.
"Enabling this feature will gives users access to the new Terra Workflow Repository UI. This replaces the Broad Methods Repository UI (and doesn't affect any workflows previously created).", | |
"Create and edit workflows with the built-in Terra Workflow Repository UI. Changes made in Terra are reflected in the external Broad Methods Repository, and vice-versa.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this? This also conveys that Broad Methods Repository is being replaced. 🤔
Enabling this feature will allow creating and editing workflows with the built-in Terra Workflow Repository UI. This replaces the external
Broad Methods Repository. But changes made in Terra are reflected in the external Broad Methods Repository, and vice-versa.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me!
@@ -291,7 +291,7 @@ export const WorkflowsContainer = (props: WorkflowContainerProps) => { | |||
}), | |||
permissionsModalOpen && | |||
h(PermissionsModal, { | |||
versionOrNamespace: 'Version', | |||
versionOrCollection: 'Version', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incredibly glad we avoided working on snapshot => version and namespace => collection at the exact same time.
@@ -77,15 +77,15 @@ const createWorkflowModalConstraints = { | |||
}, | |||
}; | |||
|
|||
interface NamespaceNameSectionProps { | |||
interface CollectionAndNameSectionProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to also modify namespace to collection inside the interface as well? Also what does name refers to here, the collection name or the workflow name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had intentionally decided to rename only user facing and component names to collection
because we had made a decision in previous renaming efforts to only change high level component names and user-facing places to new terminology. It also wouldn't be trivial to update all the instances of namespace
to collection
everywhere in code. And even if we do change the variable names we would still need to refer these as namespace (and snapshot) whenever we are using the Ajax calls so that it can be consistent with backend terminology.
Having said that, I do understand that there might be some confusion when someone else comes and looks at the code. So I am open to the discussion on renaming what else what be good for developer readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, in that case maybe a little comment in the code explaining that a namespace is the collection, and name is the workflow name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I can do that. I will also update the component name to be CollectionAndWfNameSection
so it is more clear.
@@ -118,7 +118,7 @@ describe('PermissionsModal', () => { | |||
}); | |||
|
|||
// ASSERT | |||
expect(screen.getByText('Edit permissions for namespace my-namespace')); | |||
expect(screen.getByText('Edit permissions for collection my-namespace')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe change my-namespace to my-collection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my-namespace
is what is being passed on L111. I didn't rename instances of namespace to collection here since this is a test file.
@@ -166,7 +166,7 @@ const CurrentUsers = (props: CurrentUsersProps) => { | |||
}; | |||
|
|||
export const PermissionsModal = (props: WorkflowPermissionsModalProps) => { | |||
const { versionOrNamespace, namespace, setPermissionsModalOpen, refresh, permissionsProvider } = props; | |||
const { versionOrCollection, namespace, setPermissionsModalOpen, refresh, permissionsProvider } = props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how hard would it be to also change namespace to collection here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't this would be hard, but if we do decide to change variable names in this file it would be good to change it in other files as well to stay consistent, and that would not be trivial.
@@ -238,11 +238,11 @@ export const PermissionsModal = (props: WorkflowPermissionsModalProps) => { | |||
}); | |||
|
|||
const modalTitle = | |||
versionOrNamespace === 'Version' ? 'Edit version permissions' : `Edit permissions for namespace ${namespace}`; | |||
versionOrCollection === 'Version' ? 'Edit version permissions' : `Edit permissions for collection ${namespace}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the goal is to change only what is user facing, but I am afraid that the next dev in this code base might be confused by what a namespace is if it is not consistent
Quality Gate passedIssues Measures |
Jira Ticket: https://broadworkbench.atlassian.net/browse/AN-149
Summary of changes:
What
namespace
tocollection
: I have updated only the component names and user-facing instances tocollection
Why
Testing strategy
Visual Aids
Feature Preview change
Namespace -> Collection renaming