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

Implement document deletion #17

Merged
merged 9 commits into from
Mar 31, 2016
Merged

Implement document deletion #17

merged 9 commits into from
Mar 31, 2016

Conversation

ethankent
Copy link
Contributor

Related to this issue - #3

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 86.856% when pulling b75c882 on ethankent:master into 5a4afa0 on pinax:master.

@ethankent
Copy link
Contributor Author

I didn't realize I'd need to do testing for coveralls. I'll take a look at that and update.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 86.856% when pulling 8c36bb2 on ethankent:master into 5a4afa0 on pinax:master.

@@ -273,6 +273,9 @@ class DocumentDelete(LoginRequiredMixin, DeleteView):
model = Document
success_url = reverse_lazy("pinax_documents:document_index")

def get_template_names(self):
Copy link
Member

Choose a reason for hiding this comment

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

@ethankent Can we just specify the the template_name on the class directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @paltman

Thanks for the update. Just wanted to clarify your question a bit. I think you're asking if the template name can be specified in the same way that template_name is specified in the DocumentDetail class. The answer is yes!

For some reason I was thinking that the template name needed to be calculated dynamically (for instance showing a different template for post() than for get()). I suppose if we wanted to show a confirmation message that might be a good way to go. But we don't, we want to redirect upon successful deletion. So, specifying the static class variable template_name seems like a no-brainer now. Thanks for the feedback on that. I'll update right away.

@paltman
Copy link
Member

paltman commented Mar 31, 2016

@ethankent looks great. Be merged your PR on the theme and left a comment about one small thing I'd like to see fixed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 87.189% when pulling be57197 on ethankent:master into 5a4afa0 on pinax:master.

@paltman paltman merged commit a2e1ae8 into pinax:master Mar 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants