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

Owned Groups Scaffolder Field Extension #17608

Merged
merged 17 commits into from
Jun 13, 2023

Conversation

Parsifal-M
Copy link
Collaborator

@Parsifal-M Parsifal-M commented May 2, 2023

Hey, I just made a Pull Request!

Hey Team 👋

This is my attempt at adding a field extension that is a drop-down that displays only the group entities a user entity belongs to.

For now, I will keep it as a draft as there are some todos and likely some corrections to be made, I would love some feedback as I have never done this before. 🙏

  • Add some Tests
  • Probably some corrections based on feedback and pipeline results

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@github-actions github-actions bot added the area:scaffolder Everything and all things related to the scaffolder project area label May 2, 2023
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented May 2, 2023

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-scaffolder plugins/scaffolder minor v1.13.2-next.2

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

Uffizzi Preview deployment-24014 was deleted.

@Parsifal-M
Copy link
Collaborator Author

@benjdlambert FYI, no rush on this but would love some feedback as I've not done something like this before in this repo and I am sure there are better ways/things to improve. 👍

Thanks!

Copy link
Member

@benjdlambert benjdlambert left a comment

Choose a reason for hiding this comment

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

Overall I think we're on the right lines, just a comment about what data to use.

const identityApi = useApi(identityApiRef);
const { loading, value: identityRefs } = useAsync(async () => {
const identity = await identityApi.getBackstageIdentity();
return identity.userEntityRef;
Copy link
Member

Choose a reason for hiding this comment

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

So I think this would be more of a GroupsImPartOfPicker, not ownership as such. It could be implemented that way in the auth-backend, but I would recommend using the ownershipEntityRefs from identity as the groups, and then you don't need the entity picker at all, you can just render a dropdown select with these options.

You might want to go and make a request to the catalog for each of the refs in the ownershipEntityRefs however to go get their metadata.title etc to display in a nice way.

Copy link
Member

Choose a reason for hiding this comment

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

Also to be clear, maybe there is a use case for this picker, so maybe we could also have the OwnedGroupsPicker one and also the UsersMemberOfGroup picker (names need work) 😂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Awesome! Thanks for the feedback, will get on this and report back 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did a ..small.. refactor on this but still working on it 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @benjdlambert,

Could I get another sanity check on this to just make sure I am moving in the right direction?

Thanks!

@Parsifal-M
Copy link
Collaborator Author

Hey, sorry for little movement on this I've been off sick so not had any time to work on it 😢 will pick it up sometime this week again.

@Parsifal-M
Copy link
Collaborator Author

Just quick update I am on holiday at the moment so will be little progress this week 🌴

Copy link
Member

@benjdlambert benjdlambert left a comment

Choose a reason for hiding this comment

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

This is looking pretty good I think - just a few comments.


export { GroupsImPartOfPickerSchema };

export const GroupsImPartOfPicker = (props: GroupsImPartOfPickerProps) => {
Copy link
Member

Choose a reason for hiding this comment

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

Still a little unsure about this name 😂 it could be something like OwnershipEntityRefPicker or something. Do you have any thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm terrible with names 😂, OwnershipEntityRefPicker this sounds good to me 👍

<InputLabel>{title}</InputLabel>
<Select value={selectedGroup} onChange={handleChange} label={title}>
{identityRefs?.map((ref: string) => (
<MenuItem key={ref} value={ref}>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure that this should be a MenuItem component? I think there might be something else that it should be.

Copy link
Member

Choose a reason for hiding this comment

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

Could be <Option> or something I feel like we might have used before. You can take a look at the other FieldExtensions :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do! Think I got a bit confused somewhere around here 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, now using Autocomplete which seems to be the norm(?)

<FormControl variant="outlined" margin="dense" required={required}>
<InputLabel>{title}</InputLabel>
<Select value={selectedGroup} onChange={handleChange} label={title}>
{identityRefs?.map((ref: string) => (
Copy link
Member

Choose a reason for hiding this comment

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

I think that we might want to go and call the catalog with these refs to decorate them with their correct name or title if thats available rather than just having group:my-group or whatever.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Think I've managed to do this 💪 I get team-a now instead of the triplets

@Parsifal-M Parsifal-M force-pushed the Owned-Groups-Field-Extension branch from b549fbc to 4f14981 Compare May 23, 2023 10:52
@Parsifal-M
Copy link
Collaborator Author

Hey @benjdlambert

Sorry for being slow on this, I think with my latest commit it's almost there, and I'd like to just maybe try and write a few tests to accompany it.

Hopefully, get it finished this week 😓

@benjdlambert
Copy link
Member

@Parsifal-M I think this is looking pretty good - would be good to test it out with some different use cases, it might be that we should drop the memberOf query, and just use the ownershipEntityRefs as they should be enough, but we can see!

@Parsifal-M Parsifal-M force-pushed the Owned-Groups-Field-Extension branch from 4f14981 to f657aca Compare May 30, 2023 07:51
@Parsifal-M Parsifal-M marked this pull request as ready for review May 30, 2023 13:09
@Parsifal-M Parsifal-M requested a review from a team as a code owner May 30, 2023 13:09
@Parsifal-M Parsifal-M requested a review from jhaals May 30, 2023 13:09
@Parsifal-M
Copy link
Collaborator Author

Hey @benjdlambert,

Think we are at the finishing line here ... thankfully!

QQ -> I need to run yarn build:api-reports I guess as the pipeline complains about it? And would it be okay to sanity-check my tests? Honestly was harder to write the test than to actually implement the field 🤣

Again sorry this took so long, actually I am implementing Backstage where I work by myself and having to customize quite a few things so time is a bit of a luxury for me 😅

@benjdlambert
Copy link
Member

Yep - we did some hacking on a test framework for FieldExtensions but it's not ready yet for OpenSourcing but it's something that's definitely going to help making field extensions! You'll need to run yarn build:api-reports locally and commit the changes. And you'll also need a changeset too, so yarn changeset and it's a minor bump I think! 👍

@Parsifal-M Parsifal-M force-pushed the Owned-Groups-Field-Extension branch from 24fd6b3 to fb7d4d9 Compare May 30, 2023 16:56
@Parsifal-M
Copy link
Collaborator Author

Parsifal-M commented May 30, 2023

Yep - we did some hacking on a test framework for FieldExtensions but it's not ready yet for OpenSourcing but it's something that's definitely going to help making field extensions! You'll need to run yarn build:api-reports locally and commit the changes. And you'll also need a changeset too, so yarn changeset and it's a minor bump I think! 👍

Woop! 💪

Could you possibly help me understand what I am doing wrong here? 😅

Then I think we're good!

// Warning: (ae-missing-release-tag) "OwnershipEntityRefPickerSchema" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
export const OwnershipEntityRefPickerSchema: CustomFieldExtensionSchema_2;

// Warning: (ae-forgotten-export) The symbol "OwnershipEntityRefPickerFieldSchema" needs to be exported by the entry point index.d.ts
// Warning: (ae-missing-release-tag) "OwnershipEntityRefPickerUiOptions" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
export type OwnershipEntityRefPickerUiOptions =
  typeof OwnershipEntityRefPickerFieldSchema.uiOptionsType;

EDIT Think I fixed it... 🤔 💭

Copy link
Member

@benjdlambert benjdlambert left a comment

Choose a reason for hiding this comment

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

Hey @Parsifal-M!

This is looking pretty good now, just two small nits and then I think we're good to go!

const [groups, setGroups] = useState<string[]>([]);
const [selectedGroup, setSelectedGroup] = useState<string | null>(null);

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you not make this useEffect(async() => {..}) or use useAsync instead of having to wrap up the fetchUserGroups?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @benjdlambert 👋 !

Happy to look into this and try to make this change, would it be okay if you help me understand why that would be better and maybe a small example? I don't know all the ins and outs of react hooks 😅 so just wondering why this is not an ideal approach

Thanks a lot!

Copy link
Member

Choose a reason for hiding this comment

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

It's just a little cleaner, and it will do the caching for you. It's much easier to read, and you have better control over the async promise that it's wrapped up with.

For instance you can do something like this:

const { data, loading, error } = useAsync( () => ...)

And you will get the return value, the loading state if the promise is not resolved yet, and the any error thats thrown from the promise, which is a nice pattern even if they're not used so much it's cleaner than using useEffect :)

const userIdentity = identity.ownershipEntityRefs;

if (!userIdentity) {
throw new NotFoundError('No ownership entity refs found');
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use the alertApi here to post an error message rather than throwing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did not know this was a thing, TIL!

Thanks!

@Parsifal-M
Copy link
Collaborator Author

Hey @Parsifal-M!

This is looking pretty good now, just two small nits and then I think we're good to go!

Hey! 👋

Thanks for the review, duly noted will do this probably on Friday as today and tomorrow are a bit crazy for me, but maybe I'll get a chance later (we shall see!)

Thanks!

Signed-off-by: Peter Macdonald <macdonald.peter90@gmail.com>
Signed-off-by: Peter Macdonald <macdonald.peter90@gmail.com>
Signed-off-by: Peter Macdonald <macdonald.peter90@gmail.com>
Signed-off-by: Peter Macdonald <macdonald.peter90@gmail.com>
Signed-off-by: Peter Macdonald <macdonald.peter90@gmail.com>
Parsifal-M and others added 10 commits June 13, 2023 16:59
Signed-off-by: Peter Macdonald <macdonald.peter90@gmail.com>
Signed-off-by: Peter Macdonald <macdonald.peter90@gmail.com>
Signed-off-by: Peter Macdonald <macdonald.peter90@gmail.com>
Signed-off-by: Peter Macdonald <macdonald.peter90@gmail.com>
Signed-off-by: Peter Macdonald <macdonald.peter90@gmail.com>
Signed-off-by: Peter Macdonald <macdonald.peter90@gmail.com>
…he meta for now

Signed-off-by: blam <ben@blam.sh>
Signed-off-by: blam <ben@blam.sh>
Signed-off-by: blam <ben@blam.sh>
Signed-off-by: blam <ben@blam.sh>
Signed-off-by: blam <ben@blam.sh>
@Parsifal-M Parsifal-M force-pushed the Owned-Groups-Field-Extension branch from 65bff8d to 0dcf350 Compare June 13, 2023 15:10
@benjdlambert benjdlambert merged commit 864cdac into backstage:master Jun 13, 2023
49 of 54 checks passed
@github-actions
Copy link
Contributor

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.15.0 release, scheduled for Tue, 20 Jun 2023.

@benjdlambert
Copy link
Member

Let’s give this a try!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:scaffolder Everything and all things related to the scaffolder project area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants