Searching for a specific charge isn't pulling up the incident with that charge listed

[sophieongithub]

Description

Hi team, Stephanie noticed that a filter search for the charge "publishing: revealing, disclosing or divulging grand jury information" (https://pressfreedomtracker.us/all-incidents/?charges=publishing%3A+revealing%2C+disclosing+or+divulging+grand+jury+information&categories=Arrest%2FCriminal+Charge) is not pulling up either of the incidents with that charge listed (https://pressfreedomtracker.us/all-incidents/alabama-publisher-charged-over-report-on-grand-jury-investigation/ and https://pressfreedomtracker.us/all-incidents/alabama-reporter-faces-felony-charge-for-article-on-grand-jury-investigation/). Could you look into this for us? Thank you!

Expected Behavior

No response

Browser and OS

Firefox, macOS

Urgency

Medium (don't forget about this)

Notifications

No response

[eloquence]

From what I can tell, the issue here is the comma in the charge name. I've tested that on staging; other charges with a comma can also not be retrieved in this manner.

It looks like we're splitting on comma to allow a Boolean search like this for "tresspassing" AND "conspiracy":
https://pressfreedomtracker.us/all-incidents/?charges=conspiracy%2Ctrespassing&categories=Arrest%2FCriminal+Charge

This also works on other fields, e.g., if you want incidents that target CNN and "media", you can get those:
https://pressfreedomtracker.us/all-incidents/?targeted_institutions=CNN%2CMedia

That's in conflict with using a comma for the charge name.

I don't see evidence that this comma search functionality is actually exposed to the user in any way. Are we relying on that somewhere explicitly? If not, maybe we can split on "|" instead so we preserve this functionality for future UI improvements but make existing values searchable.

[SaptakS]

From what I can tell, the issue here is the comma in the charge name. I've tested that on staging; other charges with a comma can also not be retrieved in this manner.

Yeah. You are exactly right. I reproduced this locally, and have been looking into what might be the best strategy here. We can definitely use a different separator. I am trying to figure out what all might that affect. The ManyRelationFilter was implemented 7 years ago, so I think maybe at that time we did expose it to the users?

[SaptakS]

I don't see evidence that this comma search functionality is actually exposed to the user in any way. Are we relying on that somewhere explicitly?

As far as I can tell we don't have this functionality anymore after the redesign to search by multi-values for a filter.

[eloquence]

In that case I think it would be safe to just split on | for now so we still preserve this functionality in principle, but avoid namespace conflicts - what do you think @SaptakS @harrislapiroff?

[harrislapiroff]

I think that's a reasonable proposal @eloquence—let's implement it

[SaptakS]

So this ended up being slightly more complicated, because seems like our tags and categories filtering does use the ManyRelationFilter which relies on commas. I am working on replacing that with | for splitting I am actually not sure if replacing , by | for categories and tags is a good idea, since there are too many places that are susceptible to breaking (inclusing visualizations). I am considering maybe separating the logic for either tags/categories, or fields like charges to a separate class.

These are all the filters that are eventually getting a ManyRelationFilter and hence splitting using comma:

• <ManyToOneRel: incident.incidentcharge>
• <ManyToOneRel: incident.incidentcategorization>
• <ManyToOneRel: incident.incidentpagelinks>
• <ManyToOneRel: incident.equipmentseized>
• <ManyToOneRel: incident.equipmentbroken>
• <ManyToOneRel: incident.targetedjournalist>
• <ManyToOneRel: incident.incidentauthor>
• incident.IncidentPage.targeted_institutions
• incident.IncidentPage.tags
• incident.IncidentPage.target_nationality
• incident.IncidentPage.workers_whose_communications_were_obtained
• incident.IncidentPage.politicians_or_public_figures_involved

[SaptakS]

The 2 options that we discussed and I think makes sense are these:

1. Make every filter except Tags & Categories split on |, but keep the Tags & Categories still split on , and disallow , in admin validation. This is probably the most pragmatic solution, since it is backward compatible with Tags and Categories filtered search page links. Also, it ensures that things that were working and dependent on tags & categories (e.g. visualization) still continue to work and not break. Also, Tags and Categories are the only things that have a visual break when comma separated, whereas the other filters just split in the backend, but visually they never split.
2. Make every filter into JSON objects (including in URL) and parse them in the backend: This was sugested by @chigby and I agree that this probably more robust and doesn't limit us to any characters for any filters. However this is not very backward compatible, and would need a lot of testing to ensure all the visualizations and visual elements for categories and tags still work correctly.

I am leaning towards Option 1, even though it's a slightly less robust solution. cc @freedomofpress/fpf-web-developers