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 March 14, 2018 17:36
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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yep, it can all be removed.

@stsewd
Copy link
Member Author

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

@RichardLitt RichardLitt added the PR: work in progress Pull request is not ready for full review label Mar 15, 2018
@stsewd
Copy link
Member Author

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

screenshot 2018-03-26 10 22 32

❤️

@ericholscher
Copy link
Member

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

@humitos humitos added the Status: blocked Issue is blocked on another issue label Apr 17, 2018
@ericholscher ericholscher removed the Status: blocked Issue is blocked on another issue label Apr 27, 2018
@ericholscher ericholscher merged commit 637f37d into readthedocs:master Apr 27, 2018
@ericholscher
Copy link
Member

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

@ericholscher
Copy link
Member

🙏 @stsewd

@stsewd stsewd deleted the issue2879 branch April 30, 2018 22:37
@stsewd
Copy link
Member Author

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
Labels
PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants