Add Pull Request builds page to settings

[ericholscher]

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

Closes #10411

[humitos]

I'm not sure to understand what's the context or the goal of this PR. Can you expand on the background of it?

Wrong button

[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: readthedocs/ext-theme#199

[agjohnson]

In the new dashboard, I'm trying to stick to a naming convention closer to Django's built in naming and I have been removing our prefixes like project_. For this, I'd use:

Suggested change

	template_name = "projects/project_pull_requests.html"
	template_name = "projects/pull_requests_form.html"

[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 readthedocs/ext-theme#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: readthedocs/ext-theme#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 readthedocs/common#198 should fix tests)

[agjohnson]

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

• Use form validation errors for important UI feedback #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: #11095 (comment)

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.

[agjohnson]

I also got 'UpdateProjectForm' object has no attribute 'setup_external_builds_option' loading the main setting page, which seems related.

This looks close, I think we could visit tuning up the prevalidation form pattern separately -- though it does involve coming back and fixing this form either way.

[agjohnson]

I suspect form.is_disabled isn't working correctly from the templates because of ModelForm base, but I haven't quite pinpointed that one yet.

[agjohnson]

Yeah, so far form.errors was called first, populating all the correct errors. The template logic if form.errors and form.is_disabled is correct for now.

Noted in:

• Form: prevalidation form tuning #11270

[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: Add Pull Request builds page to settings #10656 (review). You can see this in test failures too.
• I added suggestions to disable the form visually: Add Pull Request builds page to settings ext-theme#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 👍

[agjohnson]

Looks good to go for now! We can revisit this pattern with some fixes to prevalidation forms in #11270

[agjohnson]

This looks good. I noted to come back to these patterns in #11270 and update the code here.