search

OpenTechStrategies / streetcrm

StreetCRM is a free software contact management application
Other
5 stars 4 forks source link

Alphabetizing event search results, displaying basic, template tag for section #313

Closed pjsier closed 7 years ago

pjsier commented 7 years ago

This has the advanced search results for events as well as participants working, gets rid of the results dict and key to display basic search results, and adds a template tag for loading the event type.

Wanted to get some thoughts on whether updates enough for now (given the time frame) or whether it's worth cleaning up the the OrderedDict objects in earlier methods that aren't necessary now. I think it would be manageable to remove some of the earlier reference without doing a full refactor, but I could be underestimating it

Also, is the template tag is too much to just handle displaying the section on events? I couldn't think of another way that didn't overcomplicate the search functions

cecilia-donnelly commented 7 years ago

Hmm...I tested a couple "advanced" searches (e.g., Actions attended by Dante Stallworth, Actions attended by Jill Martinez) and all the actions were returned in both cases, which is incorrect. The other searches I did (for date range, organized by) worked correctly. The various Participant searches worked fine. I didn't go through every possible combination, but of the ones I tried the only failures were in "Actions attended by." So: good! Can you take a look at that case?

I think adding the new template tag is fine -- it's a little heavy, but it makes clear what's going on, which is important. It might be worth asking users how important marking "MAJOR" or "MINOR" is to them -- I don't know if they're using the "prep event" marker on a regular basis.

pjsier commented 7 years ago

@cecilia-donnelly will do! Looking at it now, I'm guessing it has something to do with how event_participants is generated, but I can take a closer look

pjsier commented 7 years ago

Ok, I think this change should address it. The event_participants variable included all attending participants as the value, but I didn't have a check in there to see if there were actually any participants pulled. This looks like it only came into play though when filtering for things like "attended by", so it should be fixed now.

Also changed the check for results length to checking to see if it's an event search, given the event list could potentially have one item

cecilia-donnelly commented 7 years ago

Meta-comment: we try to stick to the commit message guidelines in this post. Your most recent message is getting perilously long. Not a huge thing, but it's nice for those reading your messages if you follow a consistent format.

pjsier commented 7 years ago

@cecilia-donnelly gotcha, thanks for that article! Those guidelines make a lot of sense, so I'll try to stick to that format going forward

cecilia-donnelly commented 7 years ago

@pjsier Unfortunately when I search for actions with "Attended by" filled in I get an error: 

'NoneType' object has no attribute 'name'
streetcrm/streetcrm/admin.py in <lambda>, line 532

Looking at the traceback, I think results = sorted(results, key=lambda object: object.name) needs a check to make sure that object exists. (I did a search for Actions with a given Attendee that should have no results, because the participant hasn't attended any actions.)

Given how much of a pain this is, do you want to spend some time to refactor the advanced search? It seems like it would be worth it.

pjsier commented 7 years ago

It looks like the null check fixes the issue. I tried out searches for both participants and actions including some foreign key relations and it seems to work. I probably won't have the separate PR with the refactor in until later tonight, so wanted to get this in earlier.

For the search refactor, I'm planning on just including actions and participants, since it looks like institutions aren't currently included in the advanced search.

cecilia-donnelly commented 7 years ago

Thank, @pjsier! I ran a couple tests and it looks good. :) I'll do a little more testing, merge, and deploy this tomorrow.

cecilia-donnelly commented 7 years ago

I merged master into this branch to make a cleaner PR merge, but that added some extra commits (of course). Sorry for any confusion!

A couple small things which can be resolved separately:

  1. Alphabetization / sorting of participant results appears to be case-sensitive, so that lower-case names show up at the end of the results, even if they start with an early letter (e.g. "b"). I don't think this is a huge thing, since most names will likely be capitalized, but we should fix it.

  2. Rather than having a parenthetical "MINOR" next to the action name in search results, we should use the term "PREP" for consistency with the counts that show up in the search results. (I didn't think of this earlier!)