Mezzanine.forms add user field (optional) #600

Closed
wants to merge 5 commits into from

2 participants

@pahaz

No description provided.

@stephenmcd
Owner

Thanks for working on this. I like the feature overall, I think it's a great idea. A couple of things I think need tweaking though:

Firstly and easily enough, I don't think we need a setting for toggling it on and off. Let's just have it on by default.

Secondly and this one's a biggie. The code that was already in place for the entry_date field is pretty fragile. It takes on the ID of 0 and acts like a form field that can be exported and filtered against. The new user field kinda sits in the same boat as this. It too is a field on the entry model itself. It should be able to be optionally exported. It should be filterable (eg I can search for a username, just like other text-based fields). As far as I can tell the new user field kind of mimics the setup that entry_date uses, in that it takes the next ID in the opposite direction (-1) - I think that's ok. But the big problem is that it seems to set itself up quite differently than how entry_date does in terms of how it mimics acting like an addable field. I really think we need to make this consistent, which may require cleaning up some of the way entry_date works a bit.

Hope that makes sense. Happy to discuss it further. Like I said overall it's a good idea and I'd love to get this in.

@pahaz

May be add custom field_type ? Then user can add this field if needed.

It should be possible to hide information about users because some forms must be to anonymous.
May be add boolean field in form model as dont_keep_user or dont_display_user ?

@stephenmcd
Owner

Yeah I thought about that a bit too. It does solves some of the problems above:

  • We no longer have to worry about faking fields, it's a real field type
  • We don't have to worry about a setting, users can control a form by adding a user field to it

I think this is still a fair bit of work. We would now need a way to have fields that:

  • Don't appear at all in the rendered form, and have a mechanism for being populated from the request
  • Map to a related data type. I imagine we'd store the user ID for the field, but we'd want to deal with usernams (maybe more) in the export/search.

I'm also not 100% convinced being able to add this as a field is right from a usability (eg admin users) perspective. What relevance does having a user field in the field ordering have if it's never going to be rendered? None of course. What happens when someone wants to start capturing users for an existing form? All the previous field entries don't have related users, but they could have, if user was always implicitly captured on the entry model. In this regard, it feels much more like the entry_time field on the entry model - more clearly, it's a constant piece of data that can always be captured on a form (assuming the user is logged in), and has no relevance in terms of an ordered set of fields.

@pahaz

I was guided by the same considerations. I decided that for the show user_field must meet the administrator, but the data is stored forever. I understand that this decision is not the best.

One of the compromises - the creation of a special field in the form module (display_user) by default to make it false and forbid its change without special rights.

Need to consider the possibility of expansion. Forms are one of the most important parts of the site, but now does not provide an opportunity for a nice printout of results.

Version with customizable settings you do not like?

@stephenmcd
Owner

I don't feel very strongly about whether there's a setting or not. My preference would be to not have a setting, and to just have the user field always available. I don't feel like there's much point in hiding it, and there are a million settings already - we need to scrutinise adding each one. It might be worth posting this to the mailing list to see if we can get some more feedback on it from existing users.

I do feel strongly the other point though, with how these "fake" fields (user, entry_time) get loaded into the export view and treated like normal form fields. I definitely think it needs to be cleaned up - specifically the original code there, so that we can add new fake fields like this in a clean way without hard-coding them all. I'm actually happy to continue the work on this if you'd like - up to you.

One other thing to keep in mind - it's possible to actually capture username at the moment using a hidden field type, and giving it a default value, which can accept template code, so {{ request.user.username }} for the default value of the hidden field.

@pahaz

Hidden field is a bad choice, becouse need a mechanism to verify the data sent, such as checksums, and why it is needed, if you can do everything easier.

@stephenmcd
Owner

Yeah I agree, it could be manipulated by the user. Just wanted to point out the possibility - it might be fine for some people.

@stephenmcd
Owner

So we haven't gotten any feedback on the mailing list. That's OK, we can continue anyway.

So I think this is still where we left it. I'd prefer we don't use a setting to toggle the feature, but more importantly, we need to come up with a better approach for injecting entry_time and user fields as fake form fields, so that they can be exported and queried on in a consistant way, and if another case like this comes along, we can easily add more fake fields.

@stephenmcd stephenmcd closed this Mar 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment