Find out where/why rtd activates a version after building it

[stsewd]

Not sure how to classify this issue, but we kind of need to figure out why are we activating a version after building it, it may be a bug or it is by design.

Raised in #4733 (comment)

[stsewd]

I think I found it

https://github.com/rtfd/readthedocs.org/blob/a4291d17728b8df6b7a9ce57537c5d620416e485/readthedocs/projects/tasks.py#L605-L610

/cc @humitos

Still not sure why we do that 🤷‍♂️

[humitos]

@stsewd good work!

First of all, I think we don't need to send the active=True in that PATCH.

On the other hand, I come up with some questions:

• why we would build a version that it's not active?
• in case we want to build a version that it's not active, why we would like to mark it as active after building it?

@agjohnson @ericholscher you may remember some use case for this. Otherwise, I think we can remove the active argument from the PATCH.

Also, we should confirm that removing this we still can work around #4001.

[agjohnson]

I'm lost on the two points above as well. I'd say we don't want to build it at all if its inactive, and we certainly shouldn't be changing active=True.

Perhaps this was a case to catch the initial import of a project? When the latest version is maybe not active yet? Just guessing at this...

[stsewd]

I tested this locally, removing active from the patch works, and it doesn't break things like the first import, as we create the latest and stable with active=True

https://github.com/rtfd/readthedocs.org/blob/dfc8fc9eba8dc9caae171ca0b3e8f6a71594e088/readthedocs/builds/managers.py#L37-L59

The only parts where we are triggering builds on inactive versions are: on webhooks (fix in #4733), and when triggering manually a build (from django shell?).

If we want to move the check to trigger build, we need to refactor the other places, and we lose the ability to build inactive versions

[humitos]

In my opinion, I think that removing it from the PATCH is ok.

I don't think we want to move the check of the inactive version inside the trigger_build function. Do you have a different opinion?

[stsewd]

Do you have a different opinion?

Nope, same. Also, that is easier than touching the rest of the code p:

[ericholscher]

I guess I still don't understand -- what is the case that's happening where we are building a set of docs that we don't want to set to active?

[stsewd]

@ericholscher here #4672, but we have a fix for that #4733, but the active=True in this code doesn't make much sense

[ericholscher]

I guess I just don't see the bug as "building activates the version", but more that we shouldn't be triggering the build for inactive versions in that case -- I think in general if we're building docs they should be activated, but I could see the argument for that happening in the code that calls the build, not the build itself.