Add Pull Request builds page to settings

[ericholscher]

trafficstars

This is a basic form for now, but we can spruce it up before it goes live.

Closes #10411

[ericholscher]

@humitos It's adding a page in settings to configure PR builds. It's a huge feature that is important to users, so we should unhide it, and make it much more visible.

More info here: https://github.com/readthedocs/ext-theme/issues/199

[agjohnson]

I'm guessing this might leak into next week, one of us can jump in and take assignment here if this needs anything else.

The change was small enough here and I don't have much to add to this, but I'll defer to @humitos on anything else needed here.

[humitos]

The change looks good to me after talking a little more about this with Eric. However, tests and checks are failing, so I think there is no need to rush here.

[agjohnson]

I patched things up, tests are looking like they will pass.

[agjohnson]

I've just noticed that the advanced form has a bit more logic to it however, and this would be a good addition to retain:

Note the field is disabled because the project lacks and integration.

[humitos]

This PR is related to https://github.com/readthedocs/ext-theme/issues/211 that talks about adding a completely new admin page to configure the addons. Since there is an addon that's related with PRs (the external version warning), we could consider that issue here as well.

[agjohnson]

Ah yes, true. Ultimately, when addons are fully deployed for all projects, it would make a lot of sense to group all the options on a single admin page (that is, options "Enable pull request builds" and "Show pull request build notification in documentation").

I'm not sure how we'd treat the notification in the admin until then though, given we probably want that option dependent on whether or not the project has opted into the addons beta as well.

Let's keep this in mind as we work towards some sort of beta opt-in page, as perhaps it is a good option to add to this admin page. At very least we should link to the opt-in admin page from this admin page though.

[humitos]

I'm not sure how we'd treat the notification in the admin until then though, given we probably want that option dependent on whether or not the project has opted into the addons beta as well.

I created this issue to talk about this in particular: https://github.com/readthedocs/ext-theme/issues/212

[agjohnson]

I got this ready for merging last week, but I do think we should preserve how the field is conditionally enabled when there is a connected remote repo.

We could add the existing logic to the form instance here, disabling the field if there are no remote repos attached. I think I'd probably opt for raising a validation error though, as the greyed out field isn't super obvious and can be easy to miss.

The best solution would probably be doing something stronger at the template level, and conditionally disabling/showing the form when the remote repo is missing. This would give a good place to help describe the problem and point the user in the right direction. We sort of have this with the help text now, but I'm picturing some more complex UI here -- buttons linking out to resources, and perhaps make the warning a ui info message.

I'll hand this back over to @eric. Either option works great, we can always come back later and make the templates nicer.

[ericholscher]

Cool, I will take a look at how the field is disabled, and look at adding some additional info into the page to make it a nicer UX.

[ericholscher]

I updated this PR to pull over the validation logic into the PR build form. I noticed that when the field is disabled, you can't see the help_text. I wasn't sure the best pattern for showing the error validation message in the form.

@agjohnson do we have a pattern here? Should I set an error on the form or something so that it shows in a nicer way to the user? There was a pile of logic trying to figure out if the form should be disabled, so I didn't want to port that over the the templates.

(Believe https://github.com/readthedocs/common/pull/198 should fix tests)

[agjohnson]

This is a good question, and actually the work I am doing on #11095 is relevant here too.

• #11095

Help text can be helpful for giving hints to users about a particular field, but it's also really good at making forms way too noisy. So, I would like to move away from relying on help text for these sorts of scenarios, and especially a case where the form should be disabled when a condition isn't met.

Using the PR above as base, the Form instance here could implement a clean_prevalidation() that throws a validation error when the required configuration for PR builds isn't met. This surfaces as a titled, non-field error in the UI, and you can even go as far as to use form.is_disabled() to add disabled CSS class to the form element.

[ericholscher]

Using the PR above as base, the Form instance here could implement a clean_prevalidation() that throws a validation error when the required configuration for PR builds isn't met. This surfaces as a titled, non-field error in the UI, and you can even go as far as to use form.is_disabled() to add disabled CSS class to the form element.

This isn't a standard Django thing, so you're suggesting we build our own prevalidation step? I don't quite understand how that validation error would be shown in the UI -- is there an example of this somewhere?

[agjohnson]

This isn't a standard Django thing, so you're suggesting we build our own prevalidation step?

Yes, this is implemented already in the PR I linked though. The work required here is to throw a validation error from the Form, similar to this:

• https://github.com/readthedocs/readthedocs.org/pull/11095/files#diff-02d9f2e5eee6588f2a581db5e906ae16e33fa1925bb30d677d01c00f88716f14R169-R188

I don't quite understand how that validation error would be shown in the UI -- is there an example of this somewhere?

I pasted an example in that PR: https://github.com/readthedocs/readthedocs.org/pull/11095#issuecomment-1935300267

These validation errors are added as standard non-field errors. The PR also includes a validation error class that includes a custom header, which I'd also suggest here.

[ericholscher]

@agjohnson I tried to update this to use the prevalidation logic, but getting this weird error, and it's not disabling the form:

[agjohnson]

I think your code looks correct, but the form error is not displaying as I would expect. Are you using {{ form | crispy }} to display the form?

If the error display correctly, it should display the validation error header attribute value instead of Error as a header, and it shouldn't render as an array of strings.

and it's not disabling the form

This requires a class addition at the form at the moment. If we want a nice pattern here, there must be some way to do this programmatically from the form/view though.

For now, you can set .ui.disabled.form classes on the <form> element when the form should be disabled.

The first example I did using this didn't quite do what you are doing, there is only technically a form (hence just showing the errors for now):

https://github.com/readthedocs/ext-theme/blob/710cd193cf631eb03e8b003c03346298391bdf6a/readthedocsext/theme/templates/projects/import_form.html#L29-L36

[agjohnson]

The error displayed fine for me, so I think you maybe have an old base for your PR on ext-theme. What is not working is form.is_disabled, it doesn't seem like form._prevalidation_errors is populated -- not exactly sure why yet. The validation error shows, implying clean_prevalidate is called, but the validation error is not saved to form._prevalidation_errors. Much of this happens at PrevalidateForm.

[ericholscher]

@agjohnson Not really sure the best steps here? You can just push to this PR if you want changes vs. suggestions, but it sounds like it's not ready to merge?

[agjohnson]

What is blocking this currently:

• It seems the main settings page throws an exception: https://github.com/readthedocs/readthedocs.org/pull/10656#pullrequestreview-1992797713. You can see this in test failures too.
• I added suggestions to disable the form visually: https://github.com/readthedocs/ext-theme/pull/206

I didn't look into solving the bug introduced here with the main settings view. I did rebase these PRs locally to have everything up to date. If you aren't noticing the exception locally, you likely should after rebasing.

With everything rebased, the form errors were working as expected.

[ericholscher]

I believe these changes should be addressed in the updated PRs 👍