fix: mongoDB client impl issues

[kriswest]

@andypols noted a couple of implementation errors in the mongoDB client (not awaiting promises, searching for repos against the wrong field), which this PR corrects.

The error is not picked up by tests as our testing of the mongo client is inadequate. I'll open a separate issue to resolve that. In the meantime, this PR will need to be tested manually.

[netlify]

✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name	Link
🔨 Latest commit	bb8fe85
🔍 Latest deploy log	https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68b05df2572dcd00083a2f43

[codecov]

Codecov Report

❌ Patch coverage is 38.46154% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.76%. Comparing base (59cda2f) to head (bb8fe85).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
src/db/mongo/repo.ts	0.00%	4 Missing ⚠️
src/db/mongo/users.ts	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1167      +/-   ##
==========================================
+ Coverage   82.70%   82.76%   +0.06%     
==========================================
  Files          66       66              
  Lines        2781     2780       -1     
  Branches      333      332       -1     
==========================================
+ Hits         2300     2301       +1     
+ Misses        432      431       -1     
+ Partials       49       48       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[andypols]

Looks good, although you missed an await on the find in getUsers

[kriswest]

@andypols thanks, fixed. Had a brief poke at why TypeScript isn't helping catch these, a tweak to the toClass helper function has improved, but not resolved that situation. It probably needs removing and replacing with use of constructors in the individual classes.

[kriswest]

@finos/git-proxy-maintainers this is ready for review and merge - happy to try and get that next RC out (perhaps tomorrow) when it and one other PR are in.

[jescalada]

@kriswest I'll do some manual testing with a MongoDB setup for this one first thing tomorrow. Just want to make sure nothing is missing 🙂

[jescalada]

Tested most of the flows with MongoDB Atlas:

• Creating new repo
• Fetching all repos
• Logging in through OIDC (user creation)
• Adding user to push/approval list
• Pushing to repo
• Fetching all pushes
• Approving a push

Everything seems to be working fine! 🎉

I did find a minor issue on the pushes table:

<TableCell>
  {isGitHub && (
    <a
      href={`https://github.com/${c.committer}`}
      rel='noreferrer'
      target='_blank'
    >
      {c.committer}
    </a>
  )}
  {!isGitHub && <span>{c.committer}</span>}
</TableCell>

The committer is Juan Escalada with a space in the middle which makes the a tag rendering fail. This might be due an issue with the OIDC implementation, but I think that commitData.committer is set to be the Git user.name rather than the GitHub username.

Any thoughts on this?

[kriswest]

@jescalada thats interesting - that's an existing flaw based on the assumption that the committer name would be set to the GitHub id, which is not the case in Github's own getting started instructions: https://docs.github.com/en/get-started/git-basics/setting-your-username-in-git#setting-your-git-username-for-every-repository-on-your-computer

Looking around, its not easy to get the GitHub id from the email address - at best you can search against the single, optional public/primary email address with the API. E.g. This works: https://api.github.com/search/[email protected] but this doesn't https://api.github.com/search/[email protected]

This lib offers an alternative based on searching for commits with the email address: https://github.com/sindresorhus/github-username
I've not tried it, but it might provide a solution

Alternatively, we just drop that link and instead render the email address, with a mailto: link? That might actually be more useful that the github profile link! WDYT?

[jescalada]

@kriswest I lean towards whatever is easiest to implement and won't cause issues down the line due to edge cases 😃

The GitHub username is nice to have, I just don't understand why this isn't being recorded/extracted correctly in GitProxy (we still have that confusing "gitAccount" variable and endpoint which could be clarified).

By the way, I tested with the regular account and still get that "Juan Escalada" name issue, so it doesn't seem like GitProxy is trying to resolve the GH username to begin with. Maybe plugging it into the commitData (by looking in the user database perhaps) could be a solution?

[kriswest]

There are two places with this issue I think, ui/views/OpenpushRequests/components/PushesTable.tsx and ui/views/PushDetails/PushDetails.tsx. These should be using the same util function to render, but are not. Both are assuming the committer is set the the username at that platform - although only one of them supports that for Gitlab. In neither case is it a safe assumption. There is no attempt to resolve to the username through the gitAccount fields from our users tables (which can only support one user name and doesn't link it to a platform...).

I think changing this up to render the name and a link for the email address is the easiest and safest thing to do - the email address is really the only thing we can rely on. We can then work from there to reintroduce a way of resolving the user to either a github or gitlab user profile, with handling for failures to resolve. The resolution could either be through an API lookup of some sort (if a reliable one can be found) OR resolution through out own user table, after updating it to support usernames for multiple SCM providers.

I'll raise two issues covering that and will have a quick crack to the first (to fix the immediate rendering issues and give us email links). Let em know if you'd prefer we went another way.

[jescalada]

Sounds good to me! Feel free to merge this one in. This should be enough to make the rc2 if there aren't any other urgent PRs to get in.