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

Notify the user when deleting a superproject #5596

Merged
merged 9 commits into from May 20, 2019

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Apr 16, 2019

Closes #3742

Copy link
Member

@stsewd stsewd left a comment

Can you put a screenshot of how it looks like? Also, feels like the message should go in the template itself, rather than in python code

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Apr 24, 2019

@stsewd
Here is the screenshot.
Screenshot from 2019-04-24 23-34-54

Also, I have updated the code.

@@ -6,6 +6,11 @@
{% block content-header %}<h1>{% blocktrans with project.name as name %}Delete {{ name }}?{% endblocktrans %}</h1>{% endblock %}

{% block content %}

{% if is_superproject %}
<p>This project has subprojects under it. Deleting this will make them as regular projects. This will effect the URLs of the subprojects and they will be served normally as other projects.</p>
Copy link
Member

@stsewd stsewd Apr 24, 2019

We should put a link to the subprojects page here has subprojects...

Also, the paragraph needs to be marked for translation

'This project <a href="/dashboard/pip/subprojects/">has subprojects</a> under it. '
'Deleting this will make them as regular projects. '
'This will effect the URLs of the subprojects and they will be served normally as other projects.',
' '.join(resp_content.split()) # this is done to remove all escapte sequences and new lines characters.
Copy link
Member

@stsewd stsewd Apr 24, 2019

I think you can avoid this by using the trimmed option in the blocktrans

Copy link
Member Author

@dojutsu-user dojutsu-user Apr 24, 2019

Thank you.
I have updated the PR.

stsewd
stsewd approved these changes Apr 24, 2019
Copy link
Member

@stsewd stsewd left a comment

Thanks, I left to someone else to review the copy


response = self.client.get('/dashboard/pip/delete/')
self.assertEqual(response.status_code, 200)
self.assertIn(
Copy link
Member

@stsewd stsewd Apr 24, 2019

Copy link
Member Author

@dojutsu-user dojutsu-user Apr 24, 2019

Thank you.
This was a nice shortcut.
I have updated the PR.

Copy link
Member

@stsewd stsewd Apr 24, 2019

status code is also checked by assertContains ;)

Copy link
Member Author

@dojutsu-user dojutsu-user Apr 24, 2019

Default is already 200.
Should we write it here explictly?

Copy link
Member

@stsewd stsewd Apr 24, 2019

Default is fine

dojutsu-user and others added 3 commits Apr 24, 2019
Mention that all the subproject's URLs will break.
@humitos
Copy link
Member

@humitos humitos commented May 20, 2019

OK. I tried to do a change suggestion and I did it wrong and a proper commit was created. I updated the copy on the test as well.

I think this is ready to be merged. @stsewd do you agree?

@stsewd stsewd merged commit 79e6151 into readthedocs:master May 20, 2019
1 check passed
@dojutsu-user dojutsu-user deleted the delete-super-project branch May 20, 2019
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.

3 participants