Skip to content
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

@stsewd
Copy link
Member

@stsewd 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
Copy link
Member

@humitos 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
Copy link
Member Author

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

Copy link
Contributor

@shreyateeza shreyateeza left a comment

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

@humitos
Copy link
Member

@humitos 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
Copy link
Member

@ericholscher 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
Copy link
Member Author

@stsewd 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
Copy link
Member

@humitos humitos commented Apr 2, 2018

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

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 2, 2018

Closing then

@humitos
Copy link
Member

@humitos 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
Copy link
Member Author

@stsewd 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
Copy link
Member

@humitos 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 stsewd force-pushed the allow-hide-version-warning branch from 121cde5 to 916381f May 2, 2018
@stsewd
Copy link
Member Author

@stsewd 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
Copy link
Member

@humitos 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
Copy link
Member Author

@stsewd 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
Copy link
Member Author

@stsewd stsewd commented May 8, 2018

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

Copy link
Member

@humitos humitos left a comment

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,
Copy link
Member

@humitos humitos May 8, 2018

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

Copy link
Member Author

@stsewd stsewd May 8, 2018

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'),
Copy link
Member

@humitos humitos May 8, 2018

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?

Copy link
Member Author

@stsewd stsewd May 9, 2018

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

Copy link
Member

@humitos humitos May 14, 2018

Thanks for checking this.

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

Copy link
Member Author

@stsewd stsewd May 14, 2018


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

@humitos humitos May 8, 2018

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

Copy link
Member Author

@stsewd stsewd May 8, 2018

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'),
Copy link
Member Author

@stsewd stsewd May 9, 2018

in non-stable versions

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

Copy link
Member

@humitos humitos May 14, 2018

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

Copy link
Member Author

@stsewd stsewd May 14, 2018

Any suggestions? Maybe Show warning banner in old versions?

Copy link
Member

@humitos humitos May 15, 2018

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

Show warning banner in non-stable nor latest versions

What do you think?

Copy link
Member Author

@stsewd stsewd May 15, 2018

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

@humitos
Copy link
Member

@humitos 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 stsewd force-pushed the allow-hide-version-warning branch from 0778c8a to ad4e80e May 24, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented May 24, 2018

I just rebase and rebuild the assets

Copy link
Member

@ericholscher ericholscher left a comment

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 readthedocs:master May 24, 2018
1 check passed
@ericholscher
Copy link
Member

@ericholscher 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 allow-hide-version-warning branch May 24, 2018
@astrofrog
Copy link

@astrofrog 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
Copy link
Member Author

@stsewd 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
Copy link
Member

@humitos 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants