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

Library Panels: Add name endpoint & unique name validation to AddLibraryPanelModal #33987

Merged
merged 5 commits into from
May 14, 2021

Conversation

kaydelaney
Copy link
Contributor

What this PR does / why we need it:
Adds a byName endpoint to the library elements api in service of unique name validation for AddLibraryPanelModal.

@kaydelaney kaydelaney added this to the 8.0.0 milestone May 12, 2021
@kaydelaney kaydelaney self-assigned this May 12, 2021
@kaydelaney kaydelaney requested a review from a team as a code owner May 12, 2021 10:51
@kaydelaney kaydelaney requested a review from a team May 12, 2021 10:51
@kaydelaney kaydelaney requested review from aknuds1 and dsotirakis and removed request for a team May 12, 2021 10:51
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Is the validation for the same name in the same folder?

@kaydelaney
Copy link
Contributor Author

Is the validation for the same name in the same folder?

No, it's global. Should it be per folder?

Copy link
Contributor

@hugohaggmark hugohaggmark 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, except when you change org 😉 I left some suggestions for consideration.


// getLibraryElementByName gets a Library Element by name.
func (l *LibraryElementService) getLibraryElementByName(c *models.ReqContext) (LibraryElementDTO, error) {
return l.getLibraryElement(c, map[string]interface{}{"name": c.Params(":name")})
Copy link
Contributor

Choose a reason for hiding this comment

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

The name should be unique within an Org so I think you need org_id as a param here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right, will change

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think we need kind, and folder_id here too

@hugohaggmark
Copy link
Contributor

Is the validation for the same name in the same folder?

No, it's global. Should it be per folder?

I agree with Torkel, within the same folder, kind and org the name should be unique or should be exactly the same as the UniqueKey defined here:

{Cols: []string{"org_id", "folder_id", "name", "kind"}, Type: migrator.UniqueIndex},

@kaydelaney
Copy link
Contributor Author

Hmm, if we just want the name to be unique per folder (per kind, per org), should the name endpoint be able to return multiple library panels if several exist with the same name in different folders?

@hugohaggmark
Copy link
Contributor

Hmm, if we just want the name to be unique per folder (per kind, per org), should the name endpoint be able to return multiple library panels if several exist with the same name in different folders?

I think returning an array with elements should do it!

}, [debouncedPanelTitle]);

const invalidInput =
!isValidTitle?.value && isValidTitle.value !== undefined && panelTitle === debouncedPanelTitle && !waiting;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Settled on the use of waiting here to avoid a small bit of jank that I couldn't find a way to work around otherwise.
Initially I had wanted to use !isValidTitle.loading as one of the conditions here, but this resulted in a flash between valid/invalid styles when editing an invalid title to a valid one.
The reason this happens is that, when the user starts typing panelTitle no longer equals debouncedPanelTitle, so invalidInput becomes false, and the valid styling is used.
When the user stops typing, after 350ms debouncedPanelTitle equals panelTitle. I had hoped then, that the useAsync callback would execute and set isValidTitle.loading to true, thus keeping invalidInput false, but I suspect because of some event loop weirdness this doesn't happen for a frame or two, and so we get a flash of invalid styling before isValidTitle.loading settles on being true.
Anyway, if anyone knows a nicer way to do this, let me know.

@dprokop dprokop modified the milestones: 8.0.0-beta1, 8.0.0-beta2 May 13, 2021
@dprokop dprokop enabled auto-merge (squash) May 13, 2021 09:15
@kaydelaney kaydelaney changed the title Library Panels: Adds byName endpoint & unique name validation to AddLibraryPanelModal Library Panels: Adds name endpoint & unique name validation to AddLibraryPanelModal May 13, 2021
@kaydelaney kaydelaney disabled auto-merge May 13, 2021 09:22
Copy link
Contributor

@hugohaggmark hugohaggmark left a comment

Choose a reason for hiding this comment

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

Great work on this and works as expected 🎉

@hugohaggmark hugohaggmark added the old backport v8.0.x Mark PR for automatic backport to v8.0.x label May 14, 2021
@kaydelaney kaydelaney changed the title Library Panels: Adds name endpoint & unique name validation to AddLibraryPanelModal Library Panels: Add name endpoint & unique name validation to AddLibraryPanelModal May 14, 2021
@kaydelaney kaydelaney merged commit c778d6a into main May 14, 2021
@kaydelaney kaydelaney deleted the library-panels-qol branch May 14, 2021 14:03
hugohaggmark pushed a commit that referenced this pull request May 18, 2021
hugohaggmark added a commit that referenced this pull request May 18, 2021
…braryPanelModal (#33987) (#34285)

(cherry picked from commit c778d6a)

Co-authored-by: kay delaney <[email protected]>
@jackw jackw changed the title Library Panels: Add name endpoint & unique name validation to AddLibraryPanelModal Library Panels: Add name endpoint & unique name validation to AddLibraryPanelModal May 20, 2021
mortenaa pushed a commit to mortenaa/grafana that referenced this pull request May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants