UI changes #821
UI changes #821
Conversation
Improves the index page for attendees by adding several new columns and keeping each column its own unique data type.
Adds a property for a future UI change where badge types will be displayed in AnthroCon style.
| @@ -549,6 +549,15 @@ def unset_volunteering(self): | |||
| del self.shifts[:] | |||
|
|
|||
| @property | |||
There was a problem hiding this comment.
I can't think of a good name for this function. badge_type_with_ribbon is misleading given the actual semantics (i.e. we don't show the badge type if it's an attendee badge with a ribbon), but I really can't think offhand of a better name :/
There was a problem hiding this comment.
How about combo_badge_ribbon?
There was a problem hiding this comment.
Agree w/ ribbon_and_or_badge
|
Maybe |
|
Yeah, I once worked on a project where almost every variable had a name like (this is a real example) |
Moves the phone number field up to show before the EC phone number fields.
The Tracking function used to simply store the group or attendee name which recording changes - this changes it so it stores the uuid to guarantee unique entries. Also partially implements a created property for attendees.
Adds a way to retrieve tracking data by attendee instance, and uses it with the created and last_updated properties for attendees. This displays on their admin form.
|
@EliAndrewC I had an interesting time getting sessions to work with that last commit for pulling tracking data for attendees, can you check and make sure I'm not doing something horribly misguided? |
It turns out fk_id stores the actual id of the instance, so this switches to using that instead of changing the data stored in Tracking.
| @@ -1238,6 +1255,11 @@ def jobs_for_signups(self): | |||
|
|
|||
| def get_account_by_email(self, email): | |||
| return self.query(AdminAccount).join(Attendee).filter(func.lower(Attendee.email) == func.lower(email)).one() | |||
|
|
|||
| def get_tracking_by_attendee(self, instance, action, last_only = True): | |||
There was a problem hiding this comment.
Three things:
-
As a minor stylistic matter, it would be shorted to say
.filter_by(fk_id=instance.id, action=action). That would be equivalent to what you have. -
As a naming matter, the name
get_tracking_by_attendeeimplies that this function is only valid forAttendeeinstances, but really this would work for any model. So you should probably either restricted this to only attendees (by specifyingmodel='Attendee'in the filter, or rename this to be more generic.
Because this applies equally well to everything, I'd probably just put all three of these new functions (this one plus the two properties which use it) in the MagModel class, but that's really just future-proofing against other future uses of this and definitely doesn't nee to be done now.
- Most seriously, I think there are a couple of bugs here. this function could definitely use some unit tests, but I certainly write a lot of untested functions myself due to time constraints, so as a person whose house is made of glass I am in no position to throw stones :P
When last_only is truthy, here are the current semantics of this function:
- if there are no results, return
None - if there is exactly one result, return that result
- if there's more than one result, raise an exception
I don't think that's what you intended. As it happens, Django templates swallow this type of exception and just return a falsey value, so I think this happens to not affect you at the moment, but it would if you tried using this function in a different context.
Here's an alternate implementation which probably does what you want:
def get_tracking(self, instance, action, last_only=True):
query = self.query(Tracking).filter_by(fk_id=instance.id, action=action).order_by(Tracking.when.desc())
return query.first() if last_only else query.all()So the semantics now are as follows:
last_only=Truewill returnNoneif nothing exists and return the first item if something existslast_only=Falsewill always return a list, which will be empty if nothing exists
That's probably what you'd want in this situation.
Although be aware that saying self.session.get_tracking(...) will raise an exception if you call it on something which isn't already part of a session. Since this deals with the Tracking table, this seems reasonable, and I see that you're wrapping this stuff in an is_new check which is sensible. I just wanted to point this out for future reference.
There was a problem hiding this comment.
Huh, that's weird - I had some trouble when returning an empty query, but it works fine now. All these suggestions are in the new commit - thanks!
Moves the get_tracking functions to MagModel so they're more useful. Also adds filters to the result so they display in local time.
Makes several UI changes requested by AnthroCon.