Use RemoteRepository releation to match already imported projects
#8024
Conversation
humitos
force-pushed
the
humitos/import-project-matches
branch
from
dc7bed2 to
7e7bfe1
Compare
There was a problem hiding this comment.
This is better than the current code, but I'm still not sure this is the right approach. Just because the repo already has been imported doesn't mean the user has access, given that we don't support GH auth permissions in .org yet.
This is better than the current logic tho, so at least a step towards better modeling.
Another question: should we disable manual imports of projects that already have a project connected with that URL? That would stop non-admin users from creating RTD "forks" of projects, which might be useful at some point in the future.
|
I agree with you. This is just to fix the current bug, but it doesn't change behavior. We need to have a conversation about disabling manual import because there is a bigger issue here with translations. We currently do not support importing the same repository twice (or more times) and people have to do it via Manual Import. This is bad UX, but disabling manual import because that repository is already imported will be worse. @agjohnson has raised this problem multiple times already but we didn't work towards a good solution yet. I think this was planned to be part of the new ext-theme, but we didn't get there yet. |
| projects = Project.objects.public(user).filter( | ||
| Q(repo=self.clone_url) | | ||
| Q(repo=truncated_url) | | ||
| Q(repo=truncated_url + '.git') | | ||
| Q(repo=http_url) | | ||
| Q(repo=http_url + '.git') | ||
| remote_repository=self, | ||
| ).values('slug') |
There was a problem hiding this comment.
can we use self.project.slug here as it's an OneToOne relationship and it will reduce the number of queries if we use select_related() in the RemoteRepository Queryset?
There was a problem hiding this comment.
Oh, I think you are right! We will need to check for user permissions on that project, tho. Note that we are using .public queryset in the query here.
Can you write an example? I'm confused 😞
Also, I think eventually we will want to allow multiple projects connected to the same RemoteRepository (because of translations)
There was a problem hiding this comment.
At First, I thought we could do something similar to this
return [{
'id': self.project.slug,
'url': reverse(
'projects_detail',
kwargs={
'project_slug': self.project.slug,
},
),
}] if self.project else []But as you mentioned there are some permissions related here with .public() :(
|
Marking as blocked just to remember that we need to cherry-pick the commit in two branches. |
Currently production code consider a project already imported if the
`Project.repo` fuzzy matches `RemoteRepository.clone_url` even if that `Project`
is not linked to any `RemoteRepository`.
This allow anyone to import a repository manually using the real GitHub URL to
"take over" that repository and avoiding the real owner of that repository to
import it from "Import Project" page because it says:
This repository has already been imported
and linking to the project that someone's else has imported.
With the changes on this PR, we are showing that message only if the `Project`
is connected to a `RemoteRepository` which is only possible to be done by owners
of the GitHub repository.
humitos
force-pushed
the
humitos/import-project-matches
branch
from
7e7bfe1 to
114ad6c
Compare
Currently production code consider a project already imported if the
Project.repofuzzy matchesRemoteRepository.clone_urleven if thatProjectis not linked to any
RemoteRepository.This allow anyone to import a repository manually using the real GitHub URL to
"take over" that repository and avoiding the real owner of that repository to
import it from "Import Project" page because it says:
and linking to the project that someone's else has imported.
With the changes on this PR, we are showing that message only if the
Projectis connected to a
RemoteRepositorywhich is only possible to be done by ownersof the GitHub repository.