Allow to hide version warning #3595

stsewd · 2018-02-11T23:44:01Z

I added a boolean field to allow users hide the version warning (You are not using...). This option is per project, no per version.

This is a initial proposal, let me know if you are interested on adding this option.

Things that are missing:

Run migrations
Verify if another place needs this field (other endpoints of the api?)
Add/update tests?
Update documentation

Related feature #3547

humitos · 2018-02-15T17:19:57Z

I think the implementation that you propose here does the trick on disabling the warning, but is that what we want? For example, setting the Default branch/tag shouldn't be enough for the original use case proposed in that issue. I mean, if the user set 1.0 as default branch maybe,

when you hit 1.0 it should say nothing
on 1.1 and 1.2 it could say "This version is not released yet"

I don't know, I'm just thinking aloud. I know there are other issues related to this one that we may consider to find the general solution instead of a specific one as it is this one. We can continue discussing. For example #3547

Besides, in the issue there are other considerations like i18n and the text from the note itself.

Finally, in case that we continue this direction, this shouldn't be added to the web interface but to the yaml as a config since the web interface it's kind on a deprecation status.

stsewd · 2018-02-15T20:47:47Z

@humitos yeah, you are right, #3547 is a better solution for me. But allowing to users to choosing the level of integration of rtd could be some step here, anyway, just saying, I'm not sure if that would be a real case.

shreyateeza

@stsewd Squash your commits into one it would be better! 😄

humitos · 2018-02-20T15:19:02Z

I'm marking this as Needed: design decision since I think this is not the direction we want to go but I will leave this opened so @agjohnson or @ericholscher can make an opinion here also.

ericholscher · 2018-02-21T05:56:12Z

A couple things:

I'm generally -1 on adding new settings unless it's really important. Is this something we really need, or should we make the feature work better?
We should be adding new settings to the YAML file, not to our database. Of course, we need to keep some state in the DB around the YAML files, so that is a larger project, with more design decisions...

stsewd · 2018-02-21T16:55:57Z

Is this something we really need, or should we make the feature work better?

I think #3547 is a better improvement.

So, should I close this PR? And also we should close the related issue and link it to #3547

humitos · 2018-04-02T18:24:35Z

I wrote a comment at #3481 (comment) that, if implemented, will cover this case.

stsewd · 2018-04-02T18:55:58Z

Closing then

humitos · 2018-05-01T15:11:17Z

We had a discussion yesterday with the core team and we made a decision regarding this topic. Since we need a way to "solve" the problem that some project doesn't match our rules to decide whether to show the banner or not and we can't make those rules extemedely flexible, we decided:

save this setting in the Project db model (we will need to migrate other settings also, when full YAML configuration be ready)
mark the option as default False (so, people needs to enable it if they want to show the banner)
create a migration that set this field in True to existing projects

Basically, this PR almost fits all the needings here and we will want to continue working on this.

@stsewd would you like to wrap this PR and make it ready to be merged?

stsewd · 2018-05-01T18:00:33Z

@humitos sure

people needs to enable it if they want to show the banner

I'm don't understand this, isn't this should be the default behavior? I mean, this is one of the top features of rtd (at least I think so).

humitos · 2018-05-01T18:13:24Z

I'm don't understand this, isn't this should be the default behavior? I mean, this is one of the top features of rtd (at least I think so).

Yes, it's a really good feature... when it works :)

I mean, if the project's release rules matches our logic for the banner, it's a really good feature. Otherwise, it's annoying.

So, the default value is arguable depending on the answer to "how many project we have where this is a feature" over "how many project we have where this is a problem", I'd say. Now, how do we get those numbers?

I think the idea of making it default to False it's because, if your project doesn't follow our rules, it's broken by default. Which is not a good experience.

stsewd · 2018-05-02T06:40:52Z

create a migration that set this field in True to existing projects

I'm not sure how to do that, this is done when applying the migration on the rtd servers?

We don't have docs about this feature, so we can add them now, maybe here https://docs.readthedocs.io/en/latest/versions.html?

humitos · 2018-05-03T22:55:46Z

Yes to both.

For the migration, take a look at the django docs (I'm in the phone now) to see how to create a method that's executed when the migration is ran. You can test this locally in your instance first.

For the docs, we should add it, yes.

stsewd · 2018-05-04T14:38:06Z

@humitos I was searching for those docs, I didn't find then maybe I even didn't know what was searching 😁.

stsewd · 2018-05-08T04:33:41Z

This is ready for review, I tested the data migration and works perfectly :)

humitos

This is almost ready to merge for me. I'll appreciate if you take a look at my comments and do the changes which are not huge.

Thanks!

humitos · 2018-05-08T16:15:53Z

docs/versions.rst

+---------------
+
+This is the banner with the text *You are not using..*
+that is displayed on an unreleased version of your docs,


Is this banner displayed on latest version? I suppose that it was displayed on old releases only.

Yeah, my mistake

humitos · 2018-05-08T16:18:32Z

readthedocs/projects/migrations/0024_add-show-version-warning.py

+        migrations.AddField(
+            model_name='project',
+            name='show_version_warning',
+            field=models.BooleanField(default=False, help_text='Show warning banner in non-stable versions.', verbose_name='Show version warning'),


I think this is the only operation that we need in this PR.

The other are things that are missing. Anyway, I suppose it's safe to add them here since probably are just changes in the help_text. Would you like to check why they are here by taking a look at the previous migrations and see the differences?

Just found two things that are commented below, the rest are just help text updates

Thanks for checking this.

BTW, I saw only one comment. The one about the validator. Which is the other one?

#3595 (comment)

humitos · 2018-05-08T16:19:47Z

readthedocs/projects/migrations/0025_show-version-warning-existing-projects.py

+
+def show_version_warning_to_existing_projects(apps, schema_editor):
+    Project = apps.get_model('projects', 'Project')
+    for project in Project.objects.all():


Instead of looping for all the Projects (more than 80k), you should use the .update method here which does only one db transaction.

I was wondering how much time this would take on the rtd servers p:

stsewd · 2018-05-09T03:08:09Z

readthedocs/projects/migrations/0024_add-show-version-warning.py

+        migrations.AddField(
+            model_name='project',
+            name='show_version_warning',
+            field=models.BooleanField(default=False, help_text='Show warning banner in non-stable versions.', verbose_name='Show version warning'),


in non-stable versions

I think this should be different since the banner isn't shown in the latest version also

Yes, agree. Please, fix this in the model and here.

Any suggestions? Maybe Show warning banner in old versions?

I liked the "non-stable" since it's clear. Maybe:

Show warning banner in non-stable nor latest versions

What do you think?

I wanna to avoid the nor, but it's better than old versions

humitos · 2018-05-21T15:21:17Z

This is ready to be merge from my point of view.

We will need to be careful when doing the deploy since we are going to be updating ALL the projects.

So, before merging, maybe it worth that @agjohnson takes a quick look at it. Although, I don't want to keep blocking this for too much time.

stsewd · 2018-05-24T05:08:39Z

I just rebase and rebuild the assets

ericholscher

Looks good. We will finally get some of the DB migrations that we've been missing in as well.

ericholscher · 2018-05-24T20:18:04Z

Merging this. Need to do the migration next week, but will be good to get this out. 🍾

astrofrog · 2018-11-02T23:15:02Z

Is there any way to set this setting in the readthedocs.yml branch so that different branches/tags could have a different setting if needed?

stsewd · 2018-11-02T23:30:14Z

@astrofog no, this is a global setting, you may want to use https://github.com/humitos/sphinx-version-warning

humitos · 2018-11-03T00:48:01Z

@astrofrog let me know if you have any doubt with https://github.com/humitos/sphinx-version-warning or if it does not suit your needs. There are some examples and it's easy to setup and customize.

stsewd force-pushed the allow-hide-version-warning branch from da80e41 to 121cde5 Compare February 14, 2018 13:57

RichardLitt added the PR: ready for review label Feb 14, 2018

shreyateeza reviewed Feb 20, 2018

View reviewed changes

humitos added the Needed: design decision A core team decision is required label Feb 20, 2018

stsewd closed this Apr 2, 2018

humitos added PR: work in progress Pull request is not ready for full review and removed Needed: design decision A core team decision is required PR: ready for review labels May 1, 2018

humitos reopened this May 1, 2018

stsewd force-pushed the allow-hide-version-warning branch from 121cde5 to 916381f Compare May 2, 2018 06:13

stsewd added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels May 8, 2018

humitos requested changes May 8, 2018

View reviewed changes

humitos added PR: work in progress Pull request is not ready for full review and removed PR: ready for review labels May 8, 2018

stsewd commented May 9, 2018

View reviewed changes

stsewd added 16 commits May 24, 2018 00:04

Add show_version_warning field to Project model

71799ae

Add show_version_warning on settings form

7084eaa

Add show_version_warning to API response

1bc7320

Show version warning only if the project allow it

f1d7de7

Set show_version_warning default to false

00847ad

Add migration

728f823

Add test

e10aa03

Add data migration

4cadce6

Docs

893dfc0

Fix docs

a51bb7b

Use update

3983d7a

Remove comment_moderation field from migration

bf03a90

Static assets

9b64cba

Change help text

c04d506

Better docs

d0e84c8

Rebuild assets

ad4e80e

stsewd force-pushed the allow-hide-version-warning branch from 0778c8a to ad4e80e Compare May 24, 2018 05:07

stsewd added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels May 24, 2018

ericholscher approved these changes May 24, 2018

View reviewed changes

humitos approved these changes May 24, 2018

View reviewed changes

ericholscher merged commit fbc8aa9 into readthedocs:master May 24, 2018

stsewd deleted the allow-hide-version-warning branch May 24, 2018 22:44

stsewd mentioned this pull request May 30, 2018

Display warning banner in latest version #3547

Closed

Allow to hide version warning #3595

Allow to hide version warning #3595

Conversation

stsewd commented Feb 11, 2018

humitos commented Feb 15, 2018

stsewd commented Feb 15, 2018

shreyateeza left a comment

Choose a reason for hiding this comment

humitos commented Feb 20, 2018

ericholscher commented Feb 21, 2018

stsewd commented Feb 21, 2018 • edited Loading

humitos commented Apr 2, 2018

stsewd commented Apr 2, 2018

humitos commented May 1, 2018 • edited Loading

stsewd commented May 1, 2018

humitos commented May 1, 2018

stsewd commented May 2, 2018

humitos commented May 3, 2018

stsewd commented May 4, 2018

stsewd commented May 8, 2018

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented May 21, 2018

stsewd commented May 24, 2018

ericholscher left a comment

Choose a reason for hiding this comment

ericholscher commented May 24, 2018

astrofrog commented Nov 2, 2018 • edited Loading

stsewd commented Nov 2, 2018

humitos commented Nov 3, 2018

stsewd commented Feb 21, 2018 •

edited

Loading

humitos commented May 1, 2018 •

edited

Loading

astrofrog commented Nov 2, 2018 •

edited

Loading