Added feature for sending abandoned project mail #3722

bansalnitish · 2018-03-02T22:46:02Z

Fixes #3382
@ericholscher I have added a button which is displayed when a project meets abandon policy and a user wants to use the name of the project. I am working on linking the button to send an email when it is clicked.

stsewd · 2018-03-02T22:53:39Z

readthedocs/projects/forms.py

+            if not has_good_build:
+               latest_build_date = project.get_latest_build().date
+               today = datetime.today()
+               diff = ((today.year - latest_build_date.year) * 12 +


Is better if you use timedelta for date operations

stsewd · 2018-03-02T22:54:32Z

readthedocs/projects/forms.py

@@ -114,7 +116,25 @@ def clean_name(self):
        name = self.cleaned_data.get('name', '')
        if not self.instance.pk:
            potential_slug = slugify(name)
+            project = Project.objects.get(slug=potential_slug)
+            for user in project.users.all():
+                if user.is_superuser == True:


This can be just

if user.is_superuser: ...

stsewd · 2018-03-02T22:58:50Z

Can you add a screenshot of how this looks?

Also, I think it's better if you put a property to the project model, something like is_abandoned, so we can uses this on others part of the code.

bansalnitish · 2018-03-02T23:14:02Z

@stsewd Here is a screenshot:

Also, I have added the is_abandoned property to the project model itself!

bansalnitish · 2018-03-03T16:31:24Z

@ericholscher @stsewd I have added a feature for sending mail to the owner of abandoned projects when renaming is required. If a user enters a name which is already used and the project is abandoned, a button is displayed on top to send mail.

If the user clicks on the button mail will be sent to the owner and user will be redirected to the import page with a Mail sent message.

stsewd · 2018-03-03T22:41:43Z

readthedocs/projects/urls/private.py

@@ -103,6 +103,10 @@
    url(r'^(?P<project_slug>[-\w]+)/advertising/$',
        ProjectAdvertisingUpdate.as_view(),
        name='projects_advertising'),
+
+    url(r'^import/manual/send_mail/$',


Not sure if this should live on the project import page. I mean in general, not just the url.

But I do like this hint when the project name is already taken

Can you suggest something if we do not want this to stay on project import page ?

Maybe showing this on the project page that is abandoned (only for logged users, of course). And a link to that project on the import page if the name is already taken by an abandoned project. Just a personal suggestion anyway, I'm don't know if the core maintainers want to go in that direction here.

Also some tests for the is_abandoned property would be awesome.

Hi @stsewd , Shall I make the changes for displaying the option of sending mail as suggested by you ?
Also can you please elaborate on the tests which will help reduce false results on the is_abandoned property.
Thanks

From what Eric said

I think we should keep it light weight in terms of implementation so this isn't a lot to maintain. Perhaps just an admin-only button that says "email project owner with our abandoned project email" as a start, which sends an email from a template that we have in the repo.

I think this button must be only shown to admins (rtd admins) and probably should be on the project page.

About the tests, you can create a project with different scenarios like failed build but not too old, too old but with successful build, etc. Probably the tests should live on this file https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/rtd_tests/tests/test_project.py

Okay Got it !
Thank you

bansalnitish · 2018-03-04T19:53:12Z

@stsewd How to run the tests locally ? I was writing the tests as said by you but was not able to run them locally.

stsewd · 2018-03-04T20:06:08Z

@bansalnitish You can check https://docs.readthedocs.io/en/latest/tests.html, let me know if something isn't clear.

bansalnitish · 2018-03-04T21:58:51Z

Hi @stsewd, I have written the tests. Can you please have a look and suggest improvements!
Thanks 😄

stsewd · 2018-03-05T23:12:09Z

readthedocs/templates/core/project_bar_base.html

+            <input type="submit" name="submit-btn" value="{% trans "Email project owner with our abandoned project email" %}"/>
+          </form>
+        {% else %}
+          <p><strong>{%trans "Abandonment mail is sent to the owner of the project." %}</strong></p>


stsewd · 2018-03-05T23:19:13Z

readthedocs/projects/views/private.py

+    proj_name = project_slug
+    for user in project.users.all():
+        if AdminPermission.is_admin(user, project):
+            email = user.email


Can this be done on another way (not sure how we can decide to which owner send the email)? Probably a break is needed here.

Also I think this is insecure, since any logged in user can send a request here, right?

I made this button to be visible only to a superuser in the html template. Does that look fine?

Also, I have added a break statement. Should the mail be sent to a list of all users?

Yes, but any user could do a manual request, so probably is worth to check for the superuser here. I'm not sure what is the best way of do it anyway, probably the required_login decorator accepts a argument or there is a super_user_only decorator?

Maybe @humitos can help us here.

Sure @stsewd! Thanks 😄

@stsewd I tested this on my local instance and it works as expected!

@ericholscher Can you please guide me here!

This looks like it is checking for project owners who are admins. We just want to send the email to all project owners, but only let admins do it. We should be doing a request.user.is_superuser or similar check to determine if the logged in user is an admin, and then send the email to all project owners.

The button for sending mail is only visible to the admins i.e. I have done a request.user.is_superuser check in the html template. Also, we can check for superuser using a decorator like this: https://stackoverflow.com/questions/12003736/django-login-required-decorator-for-a-superuser. Does that look fine @ericholscher?

ericholscher

This looks like a great change! I had a few small pieces of feedback, which need to be updated before we ship it, but I think this is close!

ericholscher · 2018-03-08T10:41:59Z

readthedocs/projects/forms.py

+                project_url = project.get_absolute_url()
+                if project.is_abandoned:
+                    err_msg = ('Invalid project name, a <a href="{}" style="color: red">'
+                               'project</a> already exists with that name').format(project_url)


I don't think we need to check for abandoned, we should just always link to the project here.

ericholscher · 2018-03-08T10:43:25Z

readthedocs/projects/models.py

+    @property
+    def is_abandoned_mail_sent(self):
+        """Is abandoned mail sent."""
+        return self.abandoned_mail_sent


I don't believe this is necessary, you can just use obj.abandoned_mail_sent directly.

ericholscher · 2018-03-08T10:44:00Z

readthedocs/projects/urls/private.py

@@ -36,6 +36,10 @@
        private.project_manage,
        name='projects_manage'),

+    url(r'^(?P<project_slug>[-\w]+)/send_mail/$',
+        private.send_mail,
+        name='send_mail'),


This should have a more explicit name & URL, send_abandoned_mail or similar

ericholscher · 2018-03-08T10:46:18Z

readthedocs/projects/views/private.py

+    proj_name = project_slug
+    for user in project.users.all():
+        if AdminPermission.is_admin(user, project):
+            email = user.email


This looks like it is checking for project owners who are admins. We just want to send the email to all project owners, but only let admins do it. We should be doing a request.user.is_superuser or similar check to determine if the logged in user is an admin, and then send the email to all project owners.

bansalnitish · 2018-03-08T11:15:19Z

Hi, @ericholscher I have made the requested changes. Can you have a look once!
Thanks!

agjohnson

I started reviewing this, but I suppose I don't agree with the underlying ticket or with this feature addition. I do have specific feedback on the implementation, but I think we need to step back from this feature first. This UI will actually never be shown, as a user still needs to contact us to start the process of determining that a project is abandoned. Once that process happens, the reporting user will be assigned the namespace. So I think we're solving the wrong problem here.

There are pieces here that I think we can use, but i don't think we've well defined exactly what we want here yet.

I'm going to block on this review for now while I raise some thoughts in the underlying issue.

agjohnson · 2018-06-08T20:47:40Z

As per feedback in #3382, I'm closing this. The branch will still be available for reference if needed.

stsewd reviewed Mar 2, 2018

View reviewed changes

bansalnitish force-pushed the abandon-proj branch 4 times, most recently from 1fe2e29 to 1b4f62f Compare March 2, 2018 23:45

bansalnitish changed the title ~~WIP: Add feature for sending abandoned project mail~~ Added feature for sending abandoned project mail Mar 3, 2018

bansalnitish force-pushed the abandon-proj branch 9 times, most recently from 5fac5f0 to 8c850a9 Compare March 3, 2018 17:45

feature to send abandoned project mail

bb6e9b7

bansalnitish force-pushed the abandon-proj branch from 8c850a9 to bb6e9b7 Compare March 3, 2018 17:57

RichardLitt added the PR: ready for review label Mar 3, 2018

stsewd reviewed Mar 3, 2018

View reviewed changes

bansalnitish force-pushed the abandon-proj branch 7 times, most recently from c2dd6d7 to afc6d25 Compare March 4, 2018 16:04

bansalnitish force-pushed the abandon-proj branch from afc6d25 to 91bb197 Compare March 4, 2018 16:09

bansalnitish added 2 commits March 5, 2018 03:22

add link for sending mail on project page

f8b862b

add test

38ef44d

bansalnitish force-pushed the abandon-proj branch from 91bb197 to 38ef44d Compare March 4, 2018 21:53

fix test

a4974af

stsewd reviewed Mar 5, 2018

View reviewed changes

requested changes

543e3e6

ericholscher reviewed Mar 8, 2018

View reviewed changes

bansalnitish force-pushed the abandon-proj branch 2 times, most recently from 015d82f to 1274c8f Compare March 8, 2018 11:33

requested changes

8e2f573

bansalnitish force-pushed the abandon-proj branch from 1274c8f to 8e2f573 Compare March 8, 2018 11:42

agjohnson requested changes Mar 8, 2018

View reviewed changes

bansalnitish mentioned this pull request Mar 8, 2018

Surface "abandoned" state on projects in UI #3382

Closed

agjohnson closed this Jun 8, 2018

Added feature for sending abandoned project mail #3722

Added feature for sending abandoned project mail #3722

Conversation

bansalnitish commented Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Mar 2, 2018

bansalnitish commented Mar 2, 2018 • edited Loading

bansalnitish commented Mar 3, 2018 • edited Loading

stsewd Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bansalnitish commented Mar 4, 2018 • edited Loading

stsewd commented Mar 4, 2018

bansalnitish commented Mar 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bansalnitish Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

ericholscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bansalnitish commented Mar 8, 2018 • edited Loading

agjohnson left a comment • edited Loading

Choose a reason for hiding this comment

agjohnson commented Jun 8, 2018

bansalnitish commented Mar 2, 2018 •

edited

Loading

bansalnitish commented Mar 2, 2018 •

edited

Loading

bansalnitish commented Mar 3, 2018 •

edited

Loading

stsewd Mar 3, 2018 •

edited

Loading

bansalnitish commented Mar 4, 2018 •

edited

Loading

stsewd Mar 6, 2018 •

edited

Loading

bansalnitish Mar 8, 2018 •

edited

Loading

bansalnitish commented Mar 8, 2018 •

edited

Loading

agjohnson left a comment •

edited

Loading