Opened 12 years ago

Last modified 9 years ago

#465 accepted defect

Cannot delete attachments

Reported by: Jakob Kramer Owned by:
Priority: major Milestone:
Component: programming Keywords: test
Cc: deletesoftware@…, berkerpeksag Parent Tickets:

Description

Attachments cannot be deleted without deleting the whole "medium."

Attachments (2)

465-delete-attachment.patch (8.5 KB ) - added by digital-dreamer 10 years ago.
465-unit-tests-v1.patch (5.0 KB ) - added by digital-dreamer 10 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by Aleksej, 12 years ago

Cc: deletesoftware@… added

comment:2 by Jakob Kramer, 11 years ago

Owner: set to Jakob Kramer
Status: newassigned

comment:3 by Jakob Kramer, 11 years ago

Status: assignedaccepted

comment:4 by Christopher Allan Webber, 11 years ago

Status: acceptedin_progress

Gandaro, are you working on this? I'm switching the state to in_progress, but please unclaim if not the case.

comment:5 by Jakob Kramer, 11 years ago

Owner: Jakob Kramer removed
Status: in_progressaccepted

comment:6 by digital-dreamer, 10 years ago

Hi, I added this functionality, and it seems to work well, but the main repository on gitorious doesn't allow merge requests, so how exactly does one contribute to it? I am new to the project. The main page says to submit contributions via this ticket tracker. So, it is in my repository mediagoblin/digital-dreamer-mediagoblin on gitorious in the branch 465-delete-attachment. Please check it out and send me a message in case it still needs some improvements.

by digital-dreamer, 10 years ago

Attachment: 465-delete-attachment.patch added

comment:7 by digital-dreamer, 10 years ago

PS: I see you use patch files instead of merge requests, so I added it as a patch to this ticket's attachments.

comment:8 by Christopher Allan Webber, 10 years ago

Status: acceptedreview

comment:9 by Christopher Allan Webber, 10 years ago

This looks useful... someone needs to test this patch, but it looks promising at least.

comment:10 by ShawnRisk, 10 years ago

Keywords: test added

comment:11 by Matt Molyneaux, 10 years ago

The above patch doesn't add any unit tests (unless I'm missing something?).

In particular, the delete method on MediaAttachmentFile and the media_confirm_delete_attachment view should be tested to make sure they actually do what is expected.

comment:12 by digital-dreamer, 10 years ago

moggers87 - I forgot to write the unit tests. Here they are:

by digital-dreamer, 10 years ago

Attachment: 465-unit-tests-v1.patch added

comment:13 by Matt Molyneaux, 10 years ago

I've pushed the above patches (and a commit to fix whitespace errors!!!) to https://gitorious.org/mediagoblin/moggers87-mediagoblin/commits/issue-465>

Tests pass, but I haven't properly reviewed them yet.

comment:14 by berkerpeksag, 10 years ago

Status: reviewaccepted

Reviewing https://gitorious.org/mediagoblin/moggers87-mediagoblin/commit/e8c9c20fa07db417610d2aa8579331629d11dd26:

In mediagoblin/db/models.py:

  • except OSError, error: should be except OSError as error:
  • 'Deleted Attachment entry id "{0}"'.format(self.id): You could write {0} as {}.

In mediagoblin/tests/test_attachments.py:

  • Please regroup imports like this:
# stdlib imports

# third party imports

# mediagoblin import
  • class TestAttachments: -> class TestAttachments(object):
  • assert this_storage.file_exists(attachment_path) == True: Drop == True
  • attachment.delete();: Drop ;
  • assert this_storage.file_exists(attachment_path) == False -> assert not this_storage.file_exists(attachment_path)

In mediagoblin/user_pages/views.py:

  • Please do not mix tabs and spaces.
  • if form.confirm.data is True: -> if form.confirm.data:
  • if ((request.user.has_privilege(u'admin') and request.user.id != media.uploader)): -> if (request.user.has_privilege(u'admin') and request.user.id != media.uploader)

comment:15 by berkerpeksag, 10 years ago

Cc: berkerpeksag added
Milestone: 0.8.0

comment:16 by Christopher Allan Webber, 9 years ago

Milestone: 0.8.0
Note: See TracTickets for help on using tickets.