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
feat(topology-resourcemenu): add resource menu actions #3918
feat(topology-resourcemenu): add resource menu actions #3918
Conversation
pageUrl = `add/ns/${ns}`; | ||
} | ||
const seperator = pageUrl.includes('?') ? '&' : '?'; | ||
return `${pageUrl}${seperator}component=${obj.metadata.name}&isKnativeDisabled=${obj.kind === |
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.
It's important to note URLs are tricky beasts... always use encodeURIComponent
when crafting a url with dynamic values (MDN ref)
return `${pageUrl}${seperator}component=${obj.metadata.name}&isKnativeDisabled=${obj.kind === | |
return `${pageUrl}${seperator}component=${encodeURIComponent(obj.metadata.name)}&isKnativeDisabled=${obj.kind === |
@karthikjeeyar the final designs state that there should be an |
@christianvogt I will use the Sub menu PR #3922 that you have created and align the UI with final design |
/kind feature |
fb7676a
to
7fb6858
Compare
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.
Looking good, I have a few general remarks about the coding style but the logic looks pretty good. We'll need to make sure we combine everything after we individually get our stuff in.
if (!hasApplication || !appGroup) { | ||
return `${pageUrl}${seperator}&isKnativeDisabled=true`; | ||
} | ||
return `${pageUrl}${seperator}application=${encodeURIComponent(appGroup)}&isKnativeDisabled=true`; |
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.
We also need the param contextSource
which is a string of ${versionKindGroup}/${app-name}
from your obj
. This was discovered after you started your work, but given the status of the PR I think we can wrap it into this work.
After reviewing more and understanding your goal. I think this will need to be modified by the in-context stuff @nemesis09 is working on.
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.
Right, @nemesis09 will be passing the contextSource as a queryparams
case ImportOptions.CATALOG: | ||
pageUrl = '/catalog'; | ||
break; |
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.
This is going to be annoying. The ~new
screen isn't keeping any of the params. I haven't completely dug into it yet.
const appGroup = _.get(obj, ['metadata', 'labels', PART_OF], ''); | ||
|
||
if (!hasApplication || !appGroup) { | ||
return `${pageUrl}${seperator}&isKnativeDisabled=true`; |
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's the purpose of isKnativeDisabled
?
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.
The assumption is to handle add resource only for the normal resource(D, DC, etc). Knative revisions can only be the source (never acts as a target), so if we need to disable the knative option add form, we can use this param
CATALOG = 'CATALOG', | ||
DOCKERFILE = 'DOCKERFILE', | ||
DATABASE = 'DATABASE', | ||
EVENTSOURCE = 'EVENTSOURCE', |
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.
Is this a valid option atm? Shouldn't we leave this as part of the Event Source creation path?
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.
Removed this option atm
import { TYPE_WORKLOAD } from '../const'; | ||
import { addResourceMenu } from '../../../actions/add-resources'; | ||
|
||
const addResourcesMenu = (workload) => { |
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 think workload
is a K8sResourceKind
is it? What is the type?
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.
Type is TopologyDataObject
, added it
export const EventSource = (obj: K8sResourceKind, hasApplication?: boolean): KebabOption => { | ||
const resourceModel = modelFor(referenceFor(obj)); | ||
return { | ||
path: getMenuPath(hasApplication), | ||
label: 'Event Source', | ||
href: getAddPageUrl(obj, ImportOptions.EVENTSOURCE, hasApplication), | ||
icon: <UnknownIcon />, | ||
accessReview: asAccessReview(resourceModel, obj, 'create'), | ||
}; | ||
}; |
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 think we need this today. Since EventSource creation is not likely making it in 4.4
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 will remove the event source creation option atm
import { ImportOptions } from '../components/import/import-types'; | ||
import { getMenuPath, getAddPageUrl } from '../utils/add-resources-menu-utils'; | ||
|
||
export const FromGit = (obj?: K8sResourceKind, hasApplication?: boolean): KebabOption => { |
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.
export const FromGit = (obj?: K8sResourceKind, hasApplication?: boolean): KebabOption => { | |
export const fromGit = (obj?: K8sResourceKind, hasApplication?: boolean): KebabOption => { |
Not a type, class or a component... camelCase not PascalCase please. Repeat for each of the other functions.
}; | ||
}; | ||
|
||
export const ContainerImage = (obj: K8sResourceKind, hasApplication?: boolean): KebabOption => { |
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.
You're repeating yourself quite a bit here... I think something like currying (lots of resources on the web for this concept if you want to know more) could be of use here where you store some static variables at the start and return a function that can make the actual KebabOption
at a later time (when invoked).
// From my other comment
type KebabAction = (obj?: K8sResourceKind, hasApplication?: boolean) => KebabOption;
// Currying-based type
type KebabFactory = (
label: string,
icon: React.ReactNode,
importType: ImportOptions,
) => KebabAction;
// One setup that consumes both stages
export const createKebabOption: KebabFactory = (label, icon, importType) => (
obj,
hasApplication,
) => {
const resourceModel = modelFor(referenceFor(obj));
return {
path: getMenuPath(hasApplication),
label,
href: getAddPageUrl(obj, importType, hasApplication),
icon,
accessReview: asAccessReview(resourceModel, obj, 'create'),
};
};
// Setup each option with statics values that returns a function to be
// consumed later and provide the rest of the options
export const fromGit = createKebabOption('From Git', <GitAltIcon />, ImportOptions.GIT);
export const containerImage = createKebabOption(
'Container Image',
<OsImageIcon />,
ImportOptions.CONTAINER,
);
I didn't spend any time on the variable / function / type names, so by all means adjust as you see fit. But I think this will help you repeat less in this file and give easy access to adding new flows.
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.
refactored it, definitely looks better now.
const nodeContextMenu = (element: Node) => | ||
createMenuItems(kebabOptionsToMenu(nodeActions(element.getData()))); | ||
|
||
export { workloadContextMenu, groupContextMenu, nodeContextMenu }; |
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.
Well this is an odd way to export out of this file
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.
@christianvogt 👀 Looking at you 😃
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.
Thanks for the improvement @karthikjeeyar, looks good.
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.
Don't look at me... Jeff did it :P
default: | ||
pageUrl = `add/ns/${ns}`; | ||
} | ||
const seperator = pageUrl.includes('?') ? '&' : '?'; |
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.
You could use URLSearchParams to manage all your attributes. Create it at the start and just .append
to it in some case statements and let it do it's magic. Then just searchParams.toString()
at the end to get the output:
searchParams.toString(); // "q=URLUtils.searchParams&topic=More+webdev"
@karthikjeeyar I think you missed the first case in the designs
cc @openshift/team-devconsole-ux |
@andrewballantyne I have checked with UX team, There is no right click action in the node (Doc is not updated properly), its only on group and outside group, Node action will be part of connector work that @nemesis09 is doing. Check this comment : #3918 (comment) |
This whole thing was a lot more confusing than it needed to be and not at all straight forward on the docs. Apparently yes, Christian is correct and the UX is being adjusted in the docs to match that. Carry on! 😄 |
7fb6858
to
d5b93a6
Compare
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.
Looking better, just a couple more remarks...
import { ImportOptions } from '../components/import/import-types'; | ||
import { KebabAction, createKebabOption } from '../utils/add-resources-menu-utils'; | ||
|
||
export const fromGit = createKebabOption('From Git', <GitAltIcon />, ImportOptions.GIT); |
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.
Doh, this is likely my bad. Can we actually call this method "createKebabAction" since what it returns is a function (KebabAction) that will create the option at a later time?
export const fromGit = createKebabOption('From Git', <GitAltIcon />, ImportOptions.GIT); | |
export const fromGit = createKebabAction('From Git', <GitAltIcon />, ImportOptions.GIT); |
<CubeIcon />, | ||
ImportOptions.DOCKERFILE, | ||
); | ||
export const database = createKebabOption('Database', <DatabaseIcon />, ImportOptions.DATABASE); |
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.
export const database = createKebabOption('Database', <DatabaseIcon />, ImportOptions.DATABASE); | |
export const fromDatabaseCatalog = createKebabOption('Database', <DatabaseIcon />, ImportOptions.DATABASE); |
? Since it's really just the catalog with a filter.
const nodeContextMenu = (element: Node) => | ||
createMenuItems(kebabOptionsToMenu(nodeActions(element.getData()))); | ||
|
||
export { workloadContextMenu, groupContextMenu, nodeContextMenu }; |
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.
Thanks for the improvement @karthikjeeyar, looks good.
expect(addResourceMenu).toEqual([ | ||
fromGit, | ||
containerImage, | ||
fromCatalog, | ||
fromDockerfile, | ||
database, | ||
]); |
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.
Not sure this is a helpful test... it's just coupled to the variable. Doesn't test any change in logic.
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.
Agreed, i have removed it.
let menuTitle = ''; | ||
menuTitle = getMenuPath(true); | ||
expect(menuTitle).toEqual('Add to Application'); | ||
|
||
menuTitle = getMenuPath(false); | ||
expect(menuTitle).toEqual('Add to Project'); |
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.
It's a pretty simple and straight forward check... you could just do it in two lines:
expect(getMenuPath(true)).toEqual('Add to Application');
expect(getMenuPath(false)).toEqual('Add to Project');
@@ -0,0 +1,131 @@ | |||
import { URL } from 'url'; | |||
import { getMenuPath, getAddPageUrl } from '../add-resources-menu-utils'; |
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 think you are missing a test for createKebabAction
(ie createKebabOption
), and making sure it produces a KebabOption after being invoked.
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.
add test case for createKebabAction
// Negative checks for queryparams params. | ||
expect(url.searchParams.has('preselected-ns')).toBe(false); | ||
expect(url.searchParams.has('catagory')).toBe(false); |
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.
Hmmm, I wonder if we couldn't just use something like Array.from(url.searchParams.entries()).length
to validate how many we expected it to have. That way we don't have specific tests against other path params.
Same deal for the other tests as well.
expect(url.searchParams.has('preselected-ns')).toBe(false); | ||
expect(url.searchParams.has('catagory')).toBe(false); |
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 think we can drop these two asserts... Really all we are looking for on this test is that you don't get an application param.
params.append('category', 'databases'); | ||
break; | ||
default: | ||
pageUrl = `add/ns/${ns}`; |
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.
Hmm, I probably should handle this in my PR... Since if they land there now, they'll likely lose all their params on the first navigate.
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 think we have a use case where we want to land at the +Add
page in context.
d5b93a6
to
1dc4218
Compare
it('it should return a valid kebabAction on invoking createKebabAction', () => { | ||
const { resource } = getTopologyData(MockResources, ['deployments']); | ||
const kebabAction = createKebabAction('From Git', <GitAltIcon />, ImportOptions.GIT); | ||
expect(kebabAction(resource, true)).toHaveProperties([ | ||
'label', | ||
'icon', | ||
'path', | ||
'href', | ||
'accessReview', | ||
]); | ||
}); |
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.
You use toHaveProperties
once here, and I don't think you're doing anything special here that warranted hiding the logic under the utility you created. Recommend a more direct approach, it's just an object and you know what structure you're getting (a KebabOption
) so test for that:
it('should return a valid KebabOption on invoking a new KebabAction', () => {
const { resource } = getTopologyData(MockResources, ['deployments']);
const icon = <GitAltIcon />;
const hasApplication = true;
const label = 'From Git';
const kebabAction: KebabAction = createKebabAction(label, icon, ImportOptions.GIT);
const kebabOption: KebabOption = kebabAction(resource, hasApplication);
expect(kebabOption.label).toEqual(label);
expect(kebabOption.icon).toEqual(icon);
expect(kebabOption.path).toEqual('Add to Application');
expect(kebabOption.href).toEqual(getAddPageUrl(resource, ImportOptions.GIT, hasApplication));
expect(kebabOption.accessReview).toEqual(asAccessReview(DeploymentModel, resource, 'create'));
});
Should be noted I changed your test name as well, you had a couple issues with it.
Also delete the toHaveProperties
extension at the top of the file.
I used functions calls for href
and accessReview
as you're not really testing an explicit output, you're just using what they provide.
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.
fixed it
it('should return the page url with proper queryparams for git import flow', () => { | ||
const { resource } = getTopologyData(MockResources, ['deployments']); | ||
const url = new URL(getAddPageUrl(resource, ImportOptions.GIT, true), 'https://mock.test.com'); | ||
// Check that url contains all expected params |
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.
Since you have to make a change already, can you remove all these comments as they don't attribute themselves to the test structure anymore.
You also have an issue with lint, please check that at the same time. |
1dc4218
to
e938b38
Compare
/retest |
/test frontend |
e938b38
to
6b570f9
Compare
/test frontend |
6b570f9
to
8e11037
Compare
/lgtm No approval powers here 😞 /assign @christianvogt |
@karthikjeeyar question regarding RBAC ... In the case where the user only has Read access to this project, those Add to Project/Application menu items shouldn't be added at all, similarly to how the +Add nav item is not added/shown. Can you verify that is the case? Thanks! |
@serenamarie125 Currently, For user with limited permission, I am showing all the menu items options disabled in the add menu, if we want to remove it , then should we need to keep User with view permission will see these options in Add page now, even though they dont have add permission. |
/retest |
params.append('category', 'databases'); | ||
break; | ||
default: | ||
pageUrl = `add/ns/${ns}`; |
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 think we have a use case where we want to land at the +Add
page in context.
params.append('preselected-ns', ns); | ||
break; | ||
case ImportOptions.CATALOG: | ||
pageUrl = '/catalog'; |
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.
Missing the namespace?
pageUrl = '/catalog'; | |
pageUrl = `/catalog/ns/${ns}`; |
break; | ||
case ImportOptions.CONTAINER: | ||
pageUrl = `/deploy-image/ns/${ns}`; | ||
params.append('preselected-ns', ns); |
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 is preselected-ns
used that the namespace in the URL path cannot be used for?
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.
Followed the EmptyState navigation url, but it looks like namespace in the url is enough,so removed this query params
export const fromGit = createKebabAction('From Git', <GitAltIcon />, ImportOptions.GIT); | ||
export const containerImage = createKebabAction( |
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.
nit: can we add an empty line between each export
.
Not a big deal since prettier accepts it.
const primaryResource: Node = _.find(elements, { | ||
type: TYPE_WORKLOAD, | ||
}) as Node; | ||
return [...addResourcesMenu(primaryResource.getData())]; |
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.
catalog and Database don't have rbac checks in the +Add page today.
We should be consistent.
const nodeContextMenu = (element: Node) => | ||
createMenuItems(kebabOptionsToMenu(nodeActions(element.getData()))); | ||
|
||
export { workloadContextMenu, groupContextMenu, nodeContextMenu }; |
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.
Don't look at me... Jeff did it :P
switch (type) { | ||
case ImportOptions.GIT: | ||
pageUrl = `/import/ns/${ns}`; | ||
params.append('importType', 'git'); |
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.
We should probably assume that no importType
defaults to git? It's ok to also handle being explicit.
if (!hasApplication || !appGroup) { | ||
return `${pageUrl}?${params.toString()}`; | ||
} | ||
params.append('application', encodeURIComponent(appGroup)); | ||
return `${pageUrl}?${params.toString()}`; |
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.
Simplify so that there's only a single return
statement.
8e11037
to
45f8300
Compare
/approve |
/test analyze |
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.
Tested locally. The setting of the application context in the add form is not working as this work is done in this PR #3958.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, christianvogt, karthikjeeyar, vikram-raj The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
Added custom actions to redirect (with queryparams) to import flow pages from topology
Added submenus from #3922, RBAC checks for actions, aligned it with latest UX design and added unit tests
For Users with limited permission menu items will be disabled:
Fixes: https://issues.redhat.com/browse/ODC-2602