Opened 11 years ago

Last modified 8 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."

Subtickets

Attachments (2)

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

Download all attachments as: .zip

Change History (18)

comment:1 Changed 11 years ago by Aleksej

Cc: deletesoftware@… added

comment:2 Changed 10 years ago by Jakob Kramer

Owner: set to Jakob Kramer
Status: newassigned

comment:3 Changed 10 years ago by Jakob Kramer

Status: assignedaccepted

comment:4 Changed 10 years ago by Christopher Allan Webber

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 Changed 10 years ago by Jakob Kramer

Owner: Jakob Kramer deleted
Status: in_progressaccepted

comment:6 Changed 9 years ago by digital-dreamer

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.

Changed 9 years ago by digital-dreamer

Attachment: 465-delete-attachment.patch added

comment:7 Changed 9 years ago by digital-dreamer

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 Changed 9 years ago by Christopher Allan Webber

Status: acceptedreview

comment:9 Changed 9 years ago by Christopher Allan Webber

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

comment:10 Changed 9 years ago by ShawnRisk

Keywords: test added

comment:11 Changed 9 years ago by Matt Molyneaux

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 Changed 9 years ago by digital-dreamer

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

Changed 9 years ago by digital-dreamer

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

comment:13 Changed 8 years ago by Matt Molyneaux

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 Changed 8 years ago by berkerpeksag

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 Changed 8 years ago by berkerpeksag

Cc: berkerpeksag added
Milestone: 0.8.0

comment:16 Changed 8 years ago by Christopher Allan Webber

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