Skip to content
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

Allow deep linking with both category+group, and fix various template lookups #3120

Merged
merged 3 commits into from Jul 16, 2020

Conversation

dracos
Copy link
Member

@dracos dracos commented Jul 15, 2020

Fixes #3110. And while fixing, spotted a variety of places where category lookup on a hash would fail.

@dracos dracos requested a review from struan July 15, 2020 15:44
@dracos dracos force-pushed the 3110-deep-linking-category-group branch from d5cd9bb to e42de2c Compare July 15, 2020 16:37
@davea davea self-requested a review July 15, 2020 17:58
Copy link
Member

@davea davea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better, but there are still a couple of bugs that need squashing.

When using filter_category and filter_group from the front page the selection isn't applied correctly when the user reaches /report/new; it appears that all the <option> elements that match filter_category have the selected attribute which results in the wrong top-level group being chosen.

For example if I have a 'Illegal Parking' category which exists in three groups: 'Car parking', 'Roads & pavements', and 'Test Group', and I want the user to see only the 'Car parking' group, I'd visit https://hackney.fms.davea.me/?filter_category=Illegal%20Parking&filter_group=Car%20parking

The filters on /around are displayed correctly:

image

image

image

But clicking the map pre-selects the wrong top-level category:
image

Going directly to /report/new also appears to ignore filter_group as all <option> elements that match filter_category have the selected attribute, e.g. https://hackney.fms.davea.me/report/new?longitude=-0.056961&latitude=51.545133&filter_category=Illegal%20Parking&filter_group=Car%20parking

image

image

image

@dracos dracos force-pushed the 3110-deep-linking-category-group branch from e42de2c to 62f630c Compare July 16, 2020 10:11
@dracos
Copy link
Member Author

dracos commented Jul 16, 2020

The first issue was the selection of the category from the filter (as opposed to old_category) was always selecting the last, not respecting a group. Factored that to be treated same as old category in fixup 2, more consistent now. And fixup 1 for the /report/new which adds a group check to the HTML selection of category so if present will only match a category in a matching group (similar to the filter).

@dracos dracos requested review from davea and removed request for struan July 16, 2020 10:14
@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #3120 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3120   +/-   ##
=======================================
  Coverage   83.42%   83.42%           
=======================================
  Files         248      248           
  Lines       15574    15574           
  Branches     2906     2906           
=======================================
  Hits        12992    12992           
  Misses       1660     1660           
  Partials      922      922           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a37b57...859fe13. Read the comment docs.

A hash lookup in a template is escaping the key used, meaning the lookup
is failing anywhere we are using a category containing an ampersand as a
key. A continuation of the changes made in 527d763.
If both are specified, we want to treat it as an AND, not an OR.
If a single filter item, that was in multiple groups, was selected,
then the entry in the last group was being selected in the category
dropdown when starting a new report. Make sure we check for a match
in the group first, the same behaviour as when there is an existing
category.
@dracos dracos force-pushed the 3110-deep-linking-category-group branch from 62f630c to 859fe13 Compare July 16, 2020 13:58
@dracos dracos merged commit d1dc00d into master Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable deep linking to a category group and category
2 participants