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

Allow to hide version warning #3595

Merged
merged 16 commits into from May 24, 2018

Conversation

Projects
None yet
6 participants
@stsewd
Member

stsewd commented Feb 11, 2018

Closes #1616

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

This comment has been minimized.

Member

humitos commented Feb 15, 2018

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

This comment has been minimized.

Member

stsewd commented Feb 15, 2018

@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

This comment has been minimized.

Member

humitos commented Feb 20, 2018

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

This comment has been minimized.

Member

ericholscher commented Feb 21, 2018

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

This comment has been minimized.

Member

stsewd commented Feb 21, 2018

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

This comment has been minimized.

Member

humitos commented Apr 2, 2018

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

@stsewd

This comment has been minimized.

Member

stsewd commented Apr 2, 2018

Closing then

@humitos

This comment has been minimized.

Member

humitos commented May 1, 2018

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?

@humitos humitos reopened this May 1, 2018

@stsewd

This comment has been minimized.

Member

stsewd commented May 1, 2018

@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

This comment has been minimized.

Member

humitos commented May 1, 2018

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

This comment has been minimized.

Member

stsewd commented May 2, 2018

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

This comment has been minimized.

Member

humitos commented May 3, 2018

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

This comment has been minimized.

Member

stsewd commented May 4, 2018

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

@stsewd

This comment has been minimized.

Member

stsewd commented May 8, 2018

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!

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

This comment has been minimized.

@humitos

humitos May 8, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd May 8, 2018

Member

Yeah, my mistake

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'),

This comment has been minimized.

@humitos

humitos May 8, 2018

Member

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?

This comment has been minimized.

@stsewd

stsewd May 9, 2018

Member

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

This comment has been minimized.

@humitos

humitos May 14, 2018

Member

Thanks for checking this.

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

This comment has been minimized.

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

This comment has been minimized.

@humitos

humitos May 8, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd May 8, 2018

Member

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

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'),

This comment has been minimized.

@stsewd

stsewd May 9, 2018

Member

in non-stable versions

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

This comment has been minimized.

@humitos

humitos May 14, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd May 14, 2018

Member

Any suggestions? Maybe Show warning banner in old versions?

This comment has been minimized.

@humitos

humitos May 15, 2018

Member

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

Show warning banner in non-stable nor latest versions

What do you think?

This comment has been minimized.

@stsewd

stsewd May 15, 2018

Member

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

migrations.AlterField(
model_name='project',
name='repo',
field=models.CharField(help_text='Hosted documentation repository URL', max_length=255, validators=[readthedocs.core.validators.RepositoryURLValidator()], verbose_name='Repository URL'),

This comment has been minimized.

@stsewd

stsewd May 9, 2018

Member

Here a validator was added

),
migrations.AlterField(
model_name='project',
name='comment_moderation',

This comment has been minimized.

@stsewd

stsewd May 9, 2018

Member

This needs deletion

@humitos

This comment has been minimized.

Member

humitos commented May 21, 2018

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

This comment has been minimized.

Member

stsewd commented May 24, 2018

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 ericholscher merged commit fbc8aa9 into rtfd:master May 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ericholscher

This comment has been minimized.

Member

ericholscher commented May 24, 2018

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

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

@astrofrog

This comment has been minimized.

astrofrog commented Nov 2, 2018

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

This comment has been minimized.

Member

stsewd commented Nov 2, 2018

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

@humitos

This comment has been minimized.

Member

humitos commented Nov 3, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment