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 User Edit Form bugs #235 #238

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@aditnryn
Member

aditnryn commented Apr 3, 2015

This PR aims to fix bullet 1 of #235. Please review the PR. I am not sure if this fix is the most elegent way to achieve it.

@aditnryn aditnryn changed the title from Fix #235 to Fix User Edit Form bugs #235 Apr 3, 2015

@wking

This comment has been minimized.

Member

wking commented Apr 3, 2015

On Fri, Apr 03, 2015 at 03:24:24PM -0700, Aditya Narayan wrote:

I am not sure if this fix is the most elegent way to achieve it.

Yeah, hitting the database to revert the unsaved changes doesn't seem
right, but I can't think of a better way either. Personally, I don't
think setting the title for the edit-page is worth an extra database
query, so I'd just drop this and go back to generic, model-level
titles (or just have the title be ‘amy’). If we want to keep the
entry-specific titles, this change is probably fine.

@aditnryn

This comment has been minimized.

Member

aditnryn commented Apr 3, 2015

@wking Yes, I agree with you. I am against generating database query for changing the title. I would go for generic titles, unless there is another way to achieve the same without hitting the database.

@wking

This comment has been minimized.

Member

wking commented Apr 3, 2015

On Fri, Apr 03, 2015 at 03:50:50PM -0700, Aditya Narayan wrote:

I would go for generic titles, unless there is another way to
achieve the same without hitting the database.

I'm sure there is some way to cache the original values and then
restore the object if saving it fails, but I don't think per-object
titles matter enough to make it worth figuring out ;).

@gvwilson

This comment has been minimized.

Member

gvwilson commented May 27, 2015

Are we expecting to merge this, or has it gone stale?

@pbanaszkiewicz

This comment has been minimized.

Member

pbanaszkiewicz commented May 27, 2015

I'll probably wontfix #235, but need some time to think about that issue. Should be this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment