AndrewIngram / django-extra-views

Django's class-based generic views are awesome, let's have more of them.

MIT License

1.39k stars 172 forks source link

Revert view.object back to original value if inline validation fails #186

Closed sdolemelipone closed 5 years ago

sdolemelipone commented 5 years ago

As per issue #155.

@Murfen please confirm if this resolves the issue for you.

codecov-io commented 5 years ago

Codecov Report

Merging #186 into master will increase coverage by 0.18%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
+ Coverage   50.75%   50.93%   +0.18%     
==========================================
  Files           6        6              
  Lines         530      532       +2     
  Branches       58       58              
==========================================
+ Hits          269      271       +2     
  Misses        243      243              
  Partials       18       18

Impacted Files	Coverage Δ
extra_views/advanced.py	`54.78% <100%> (+0.8%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update beaad11...7d72057. Read the comment docs.

jonashaag commented 5 years ago

Maybe we should provide another way to distinguish the cases rather than implicitly providing that state through the absence of a variable.

„Any sufficiently complicated model class contains an ad-hoc, informally-specified, bug-ridden, slow implementation of half of a state machine.“

sdolemelipone commented 5 years ago

Yes the specification "self.object is None for Create views, but is a model instance for Update views" is definitely an implicit one, even though it is originally in Django's code not ours!

As a boolean I think it would be sufficient to check self.object.pk in template languages and getattr(self.object, 'pk', None) in Python.

Templates may use self.object in rendering data on the page; for a CreateView that means self.object will be

None on a GET request.
None on a response to a PUT request if form fails to validate.
An instance of view.Model on a response to a PUT request if the form validates but the inline formsets do not.

That does seem like an inconsistent situation for template writers to deal with. And one that doesn't occur with the simpler Django CreateView.

I agree it's not a pretty change. So perhaps @Murfen could tell us if there are situations that aren't addressed by my suggestion of checking self.object.pk?

jonashaag commented 5 years ago

Ok never mind my comment about implicit state. This is a Django design issue that we ought not to deal with here

sdolemelipone commented 5 years ago

:+1: Ok thanks. I think we should leave this open for further comments before merging/closing.

jonashaag commented 5 years ago

@sdolemelipone Can we merge this? Yes right?

sdolemelipone commented 5 years ago

Hey @jonashaag, yes we can. I'll do the merge tomorrow.

sdolemelipone commented 5 years ago

Or today :-)