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

[WIP] Remove comments app #3802

Merged
merged 5 commits into from Apr 27, 2018
Merged

[WIP] Remove comments app #3802

merged 5 commits into from Apr 27, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 14, 2018

PR on top of #3618 (#3618 (comment))

Closes #2879

shreyateeza and others added 3 commits Mar 14, 2018
readthedocs.comments.models.ModerationActionManager is unimplemented. This is a cleanup issue.

Closes readthedocs#2879
@@ -136,6 +136,7 @@ class Project(models.Model):
'DirectoryHTMLBuilder">More info</a>.'))

# Project features
# TODO: remove this?
allow_comments = models.BooleanField(_('Allow Comments'), default=False)
comment_moderation = models.BooleanField(
Copy link
Member Author

@stsewd stsewd Mar 14, 2018

Are this fields part of the comment app?

allow_comments and comment_moderation.

I'm going to see if the tests explode if I remove those lines.

Copy link
Member

@humitos humitos Mar 14, 2018

Probably should be removed also. Can you grep the code by allow_comments and comment_moderation to see where it's used?

Copy link
Member Author

@stsewd stsewd Mar 15, 2018

I remove stuff related to comment_moderation, and I see that is very related to allow_comments, but I'm have doubts about this

https://github.com/rtfd/readthedocs.org/blob/aefa1b6067f87e2551d8a0e1254ec545b94008d4/readthedocs/doc_builder/backends/sphinx.py#L217-L218

Copy link
Member

@humitos humitos Mar 15, 2018

I think we can safely remove that readthedocs-comments builder and all the -comments builders.

Copy link
Member

@ericholscher ericholscher Mar 26, 2018

Yep, it can all be removed.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 15, 2018

@humitos I delete the db fields and everything looks good, should I add those migrations here or wait to #3608?

@humitos
Copy link
Member

@humitos humitos commented Mar 15, 2018

@humitos I delete the db fields and everything looks good, should I add those migrations here or wait to #3608?

I'd say to add them so it's ready to merge. Then we can squash the migration when needed.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 15, 2018

Also we want to remove the comments builder from https://github.com/rtfd/readthedocs-sphinx-ext (I'm not familiar with that repo)

@agjohnson agjohnson added this to the Cleanup milestone Mar 15, 2018
@humitos
Copy link
Member

@humitos humitos commented Mar 23, 2018

Also we want to remove the comments builder from rtfd/readthedocs-sphinx-ext (I'm not familiar with that repo)

I suppose that you can remove the whole comments folder, https://github.com/rtfd/readthedocs-sphinx-ext/tree/master/readthedocs_ext/comments

@humitos humitos mentioned this pull request Mar 23, 2018
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 26, 2018

screenshot 2018-03-26 10 22 32

❤️

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 26, 2018

I suppose that you can remove the whole comments folder, https://github.com/rtfd/readthedocs-sphinx-ext/tree/master/readthedocs_ext/comments

Yea, I have an initial PR doing this that could be improved (readthedocs/readthedocs-sphinx-ext#31)

@humitos
Copy link
Member

@humitos humitos commented Apr 4, 2018

@stsewd would you like to finish Eric's PR in the other repo so we can go forward whith this one also?

@humitos
Copy link
Member

@humitos humitos commented Apr 17, 2018

@stsewd would you like to finish Eric's PR in the other repo so we can go forward whith this one also?

Blocked by: readthedocs/readthedocs-sphinx-ext#38

@ericholscher ericholscher merged commit 637f37d into readthedocs:master Apr 27, 2018
1 check passed
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 27, 2018

Merging this, and we can figure out if it has further issues during dev today before deploying it.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 27, 2018

🙏 @stsewd

@stsewd stsewd deleted the issue2879 branch Apr 30, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 30, 2018

The only thing missing here are the migrations, I think I can add them to #3608

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants