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

#954 avoid orphaned files when using admin > advanced > replace file #958

Closed
wants to merge 1 commit into from

Conversation

SachaMPS
Copy link

@SachaMPS SachaMPS commented Feb 6, 2017

This should be a workaround for:
#954

@benzkji
Copy link
Contributor

benzkji commented Aug 29, 2017

not a real workaround, looks ok?! slightly better exception handling could be done, but then that's it - I once read one should not just except:, but always provide what you expect to except...

+1 for merging this and wasting less server space ;-)

Copy link
Member

@yakky yakky left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this, definitely much needed. I only added a minor remark, could you address it?

this = File.objects.get(id=self.id)
if this.file != self.file:
this.file.delete(save=False)
except:
Copy link
Member

Choose a reason for hiding this comment

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

It's better to explicitly handle the expected exceptions

@yakky
Copy link
Member

yakky commented Mar 30, 2018

@SachaMPS Thanks for contributing this,, could you rebase on top of current develop?
Sorry it took so long to handle this

@yakky yakky added this to the 1.4 milestone Apr 9, 2018
@benzkji
Copy link
Contributor

benzkji commented Jul 1, 2020

how about a merge...1.4 is out :-)

@jrief
Copy link
Collaborator

jrief commented Jul 2, 2020

@benzkji I fully agree with @yakky on #958 (comment)

@benzkji
Copy link
Contributor

benzkji commented Jul 2, 2020

sure!

@jrief jrief mentioned this pull request Sep 3, 2020
@stale
Copy link

stale bot commented Jul 28, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 28, 2022
@benzkji
Copy link
Contributor

benzkji commented Aug 1, 2022

is this issue resolved? anyone?

@merwok
Copy link

merwok commented Aug 1, 2022

No, this PR was not merged (author did not reply to review) and the ticket is still open.

@stale stale bot removed the stale label Aug 1, 2022
@benzkji
Copy link
Contributor

benzkji commented Aug 2, 2022

someone would need to handle the exception, and off we go, right?

@marksweb
Copy link
Member

marksweb commented Aug 2, 2022

What are the possible exceptions here. Obviously there's the DoesNotExist from the get(), but is there something else?

@benzkji
Copy link
Contributor

benzkji commented Aug 3, 2022

if this.file is none (not the usual case, but could be...), this.file.delete() would throw a "None has no method 'delete()'". Rather edge case. Dont see others...

@stale
Copy link

stale bot commented Nov 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 1, 2022
@stale
Copy link

stale bot commented Nov 29, 2022

This will now be closed due to inactivity, but feel free to reopen it.

@stale stale bot closed this Nov 29, 2022
@benzkji
Copy link
Contributor

benzkji commented Nov 30, 2022

This is important, isn't it?

@benzkji
Copy link
Contributor

benzkji commented Nov 30, 2022

@jrief or was this alreaedy merge before? It's an easy picking...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants