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

fix issue where Set.delete_uploaded_files removes files it should not… #429

Closed
wants to merge 2 commits into from
Closed

fix issue where Set.delete_uploaded_files removes files it should not… #429

wants to merge 2 commits into from

Conversation

leonelcamara
Copy link
Contributor

… during an update

The change I made differentiates between updates and deletes by taking advantage of upload_fields always being None in the case of deletes. If it is an update it only deletes the file if the field with it was updated and the new value is different.

See:
https://groups.google.com/forum/#!topic/web2py-developers/gwEr1cHQNAY

@leonelcamara
Copy link
Contributor Author

leonelcamara commented Nov 21, 2016

I'm still not sure this is the complete fix as it is possible that an update causes compute fields to change and they will not appear in upload_fields, causing the file to not be deleted.

My second commit fixes this problem however it causes some fields to be computed twice, any suggestion?

@codecov-io
Copy link

codecov-io commented Nov 21, 2016

Current coverage is 65.75% (diff: 30.00%)

Merging #429 into master will decrease coverage by 0.02%

@@             master       #429   diff @@
==========================================
  Files            69         69          
  Lines          8912       8918     +6   
  Methods           0          0          
  Messages          0          0          
  Branches       1949       1950     +1   
==========================================
+ Hits           5862       5864     +2   
- Misses         2546       2549     +3   
- Partials        504        505     +1   

Powered by Codecov. Last update 4d22bb4...edd508b

@@ -2141,24 +2148,23 @@ def delete_uploaded_files(self, upload_fields=None):
oldname = record.get(fieldname, None)
if not oldname:
continue
if upload_fields and fieldname in upload_fields and \
oldname == upload_fields[fieldname]:
continue
Copy link
Member

Choose a reason for hiding this comment

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

@leonelcamara why changing this whole block of code to avoid a continue?

@gi0baro
Copy link
Member

gi0baro commented Nov 22, 2016

@leonelcamara @mdipierro @niphlod can I start a discussion about computed fields before merging this?

I would like to change the whole logic of how computed fields are computed during inserts/updates 'cause for example we still can't use them in callbacks (see emmett-framework/emmett#151 for example). May I produce a bigger PR including this fix? What do you think?

@niphlod
Copy link
Member

niphlod commented Nov 23, 2016 via email

@gi0baro
Copy link
Member

gi0baro commented Nov 23, 2016

@niphlod what do you mean by streamlined? :)

@leonelcamara
Copy link
Contributor Author

I would like to change the whole logic of how computed fields are computed during inserts/updates 'cause for example we still can't use them in callbacks (see emmett-framework/emmett#151 for example). May I produce a bigger PR including this fix? What do you think?

@gi0baro sure, I think we need to discuss computed fields, because although this PR fixes a problem it is not efficient. Fields with a compute should probably have a required_fields for instance.

@niphlod
Copy link
Member

niphlod commented Nov 23, 2016 via email

@mdipierro
Copy link
Contributor

@leonelcamara @gi0baro this PR now has conflicts with the one that was merged. Can you please help resolve those?

@gi0baro
Copy link
Member

gi0baro commented Dec 20, 2016

@leonelcamara @mdipierro I close this since is already fixed by #431

@gi0baro gi0baro closed this Dec 20, 2016
@mdipierro
Copy link
Contributor

thanks!

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.

None yet

5 participants