-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add Build managers and Update Build Querysets. #5779
Add Build managers and Update Build Querysets. #5779
Conversation
@saadmk11 Is it possible to have this PR have a base of |
@ericholscher github only allows to do that with branches in the same repo, not for forks. So, we could push the other branch to this repo to have that feature. |
Pushed them up, you can see it here: version-update...build-manager-update |
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, a similar approach 👍
@@ -116,7 +116,6 @@ def api(self, user=None, detail=True): | |||
|
|||
class BuildQuerySet(SettingsOverrideObject): | |||
_default_class = BuildQuerySetBase | |||
_override_setting = 'BUILD_MANAGER' |
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.
We can't change this setting. We need to be able to still override this.
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.
@ericholscher actually we are now using BuildManager
not BuildQueryset.as_manager()
so i put this settings in the BuildManager.
https://github.com/rtfd/readthedocs.org/blob/3f93788ea69ad90c6230f4e9d19aae43a8428020/readthedocs/builds/managers.py#L176
3f93788
to
5607164
Compare
adfa2af
to
de5ce44
Compare
Follow up from readthedocs#5798
aa9bae6
to
5d6c750
Compare
05951cf
to
5a05b6c
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.
This looks mostly good currently. I will merge it into the feature branch here soon.
# builds in the build list page | ||
self.project_slug = self.kwargs.get('project_slug', None) | ||
self.project = get_object_or_404( | ||
Project.objects.protected(self.request.user), |
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.
We shouldn't used protected
, let's just use public
.
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.
@ericholscher This is default in the base class should we change this?
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.
@ericholscher This is default in the base class and we were using it should we change it?
Will merge this now, and we can fix up any issues later. |
This is a continuation of #5750.
Tasks:
Build
Querysets withinternal
andexternal
managers everywhere.Managers