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.

Sign up for GitHub

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

Jump to bottom

Remove unused views and their translations. #4632

Merged
merged 5 commits into from
Oct 31, 2018
Merged

Remove unused views and their translations. #4632

merged 5 commits into from
Oct 31, 2018

Conversation

ba11b0y
Copy link
Contributor

@ba11b0y ba11b0y commented Sep 14, 2018

Closes #4420
Removed unused views, their templates and translations.

@stsewd
Copy link
Member

stsewd commented Sep 14, 2018

I wonder if this was something to do with #450. But what I can tell, that was to make an alias to projects (not versions).

Also, the model used it this one https://github.com/rtfd/readthedocs.org/blob/4f1a5dc07fd9d55d4284fdb22deae735932b2ec9/readthedocs/builds/models.py#L408-L425

Looks like we aren't using that anymore. I think we can remove all related forms and models too.

@ericholscher @agjohnson do you agree or maybe this is used on the .com site?

@agjohnson
Copy link
Contributor

I don't think we are going to use alias, no. This is going to heavily conflict with my PR to update all of our translations, so perhaps it would be best to scope this PR as just removal of aliases, and not lump multiple PRs into one.

@ba11b0y
Copy link
Contributor Author

ba11b0y commented Oct 1, 2018 •
edited
Loading

I don't think we are going to use alias, no. This is going to heavily conflict with my PR to update all of our translations, so perhaps it would be best to scope this PR as just removal of aliases, and not lump multiple PRs into one.

@agjohnson So should I push a commit which reverts back to the old translations to not arise any conflicts?

@stsewd
Copy link
Member

stsewd commented Oct 1, 2018

@invinciblycool yeah, the PR that Anthony mentions was already merged

@ba11b0y ba11b0y force-pushed the refactor/remove-unused-views branch from cba3583 to 6afe227 Compare October 5, 2018 12:57
@ba11b0y
Copy link
Contributor Author

ba11b0y commented Oct 5, 2018

@stsewd Done.

stsewd
stsewd reviewed Oct 8, 2018
View reviewed changes
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove more code here, like https://github.com/rtfd/readthedocs.org/blob/90b1e52cfd6a124bf7298a1b3cb1711fddd67ab8/readthedocs/builds/forms.py#L12-L27

readthedocs/oauth/migrations/0009_auto_20180927_1049.py Outdated
@@ -0,0 +1,31 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This migration doesn't look related to this PR

@ba11b0y ba11b0y force-pushed the refactor/remove-unused-views branch 2 times, most recently from be5777e to 4b4e770 Compare October 12, 2018 11:42
@ba11b0y
Copy link
Contributor Author

ba11b0y commented Oct 12, 2018

@stsewd Please check now.

@stsewd
Copy link
Member

stsewd commented Oct 12, 2018

@invinciblycool there is more code to remove, please check my comments ^

@ba11b0y
Copy link
Contributor Author

ba11b0y commented Oct 13, 2018

@stsewd Apart from removing other occurrences, I have removed alias.pk from a test at 3bc3566. Not sure if it breaks anything.

stsewd
stsewd reviewed Oct 15, 2018
View reviewed changes
readthedocs/builds/migrations/0001_initial.py Outdated
@@ -60,16 +60,6 @@ class Migration(migrations.Migration):
'permissions': (('view_version', 'View Version'),),
},
),
migrations.CreateModel(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a new migration that removes this, not edit existing ones

@ba11b0y
Copy link
Contributor Author

ba11b0y commented Oct 17, 2018

@stsewd Pushed a commit. Sorry for the delay for this PR.

@ba11b0y ba11b0y force-pushed the refactor/remove-unused-views branch from 2812854 to 7b641a4 Compare October 17, 2018 11:01
@ba11b0y
Copy link
Contributor Author

ba11b0y commented Oct 22, 2018

@stsewd Could you please check this out.

@ba11b0y ba11b0y force-pushed the refactor/remove-unused-views branch from 7b641a4 to e9dc0c5 Compare October 22, 2018 21:09
@codecov
Copy link

codecov bot commented Oct 22, 2018 •
edited
Loading

Codecov Report

Merging #4632 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4632      +/-   ##
==========================================
+ Coverage   76.26%   76.32%   +0.06%     
==========================================
  Files         158      158              
  Lines       10034     9998      -36     
  Branches     1267     1263       -4     
==========================================
- Hits         7652     7631      -21     
+ Misses       2039     2024      -15     
  Partials      343      343
Impacted Files Coverage Δ
readthedocs/builds/models.py 75.6% <ø> (-0.38%) ⬇️
readthedocs/builds/forms.py 95.83% <100%> (+7.95%) ⬆️
readthedocs/builds/admin.py 95.83% <100%> (-0.17%) ⬇️
readthedocs/projects/views/private.py 80.1% <100%> (+1.85%) ⬆️

@RichardLitt RichardLitt requested a review from stsewd October 24, 2018 09:10
stsewd
stsewd reviewed Oct 24, 2018
View reviewed changes
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are good here, can you please rebase/merge master?

@ba11b0y ba11b0y force-pushed the refactor/remove-unused-views branch from e9dc0c5 to 736302d Compare October 24, 2018 19:27
@ba11b0y
Copy link
Contributor Author

ba11b0y commented Oct 24, 2018

@stsewd Done

stsewd
stsewd approved these changes Oct 24, 2018
View reviewed changes
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@ericholscher
Copy link
Member

In prod we only have 2 of these:


In [2]: VersionAlias.objects.all()
Out[2]: <QuerySet [<VersionAlias: Alias for Django: 1.2 -> 1.2.X>, <VersionAlias: Alias for Django: dev -> latest>]>

Guessing it should be safe to delete them.

@ericholscher ericholscher merged commit 671f4a9 into readthedocs:master Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berkerpeksag berkerpeksag berkerpeksag left review comments

@stsewd stsewd stsewd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ba11b0y @stsewd @agjohnson @ericholscher @berkerpeksag