-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Mention username who added project in a team #5128
Conversation
4120068
to
96d2ae2
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.
Many thanks! This looks like a good first pass to me 👍 I'm just a little worried about how convoluted our initial Teams & Projects code already is (re: multiple components variously enriching data one after the other).
I think a good approach to fix this would be to introduce good profiling, then identify which Teams & Projects features have the worst performance/UX impact (e.g. "fetching the list of repos is too slow"), and optimize these code paths in priority.
const owner = await this.userDB.findUserById(ownerId); | ||
if (owner) { | ||
repo.inUse = { | ||
userId : owner.id, | ||
userName: owner?.name || owner?.fullName || 'somebody' | ||
} | ||
} |
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 entire for
loop seems to add a few more DB queries to a code path that is already super slow (we already make users wait long seconds before showing anything in the UI).
I think this could possibly be optimized by refactoring projectsService.getProjectsByCloneUrls
(only used here I believe), and making that method fetch & process everything required in one go, as opposed to multiple components variously enriching data one after the other.
However, please feel free to add this as a TODO comment in the code.
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.
fully agree to this argument. it should be feasible to compute in db query.
LGTM label has been added. Git tree hash: 298a3f517afcec1020e90c337f1a359fdbfd3fb4
|
|
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.
Adding a few comments using github.dev.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Re-opening for now as it could be still relevant. Cc @jldec |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
/werft run 👍 started the job as gitpod-build-gt-mention-user-who-added-project.13 |
e4b4d6f
to
0c39e4b
Compare
Thanks @laushinka for catching issue and helping testing! |
/assign |
0c39e4b
to
04033cb
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.
Looks good - thank you @AlexTugarev @laushinka @gtsiolis and team.
{!r.inUse ? ( | ||
<button className="primary" onClick={() => setSelectedRepo(r)}>Select</button> | ||
) : ( | ||
<p className="my-auto">already taken</p> | ||
<p className="text-gray-500 font-medium"> | ||
@{r.inUse.userName} already<br/>added this repo |
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.
Mentioning here for reference - No need to implement before merging this PR:
While I was testing today, I discovered a stronger reason for doing as @gtsiolis suggested. Users with multiple integrations will be shown using the user-id of the first signup, which is not necessarily the user-id for the integration of the repo in question. E.g I used a different username for GitHub vs. GitLab and the "already added" message is showing my GitHub userid even though the repo is on GitLab.
LGTM label has been added. Git tree hash: 52f8e2bd42b68b02c09ae92dc38aa3580046d699
|
/werft run 👍 started the job as gitpod-build-gt-mention-user-who-added-project.16 |
/werft run 👍 started the job as gitpod-build-gt-mention-user-who-added-project.17 |
Co-authored-by: George Tsiolis <[email protected]> Co-authored-by: Laurie T. Malau <[email protected]> Co-authored-by: Alex Tugarev <[email protected]>
04033cb
to
ed54059
Compare
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-gt-mention-user-who-added-project.19 |
@jldec The builds were failing but turns out |
/lgtm |
LGTM label has been added. Git tree hash: 8e0fa246a36ccfc39e1d4c55f2c727a050a282ab
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Foge9627, jankeromnes, jldec Associated issue: #5119 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 |
What does this PR do?
This is using the existing structure for indicating already imported projects instead of the tooltip shown in the issue description, see #5119.
Fix #5119.