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

Shareable accurate datatable links #5098

Merged
merged 4 commits into from
Mar 15, 2022

Conversation

johnnyporkchops
Copy link
Contributor

@johnnyporkchops johnnyporkchops commented Mar 7, 2022

Summary (required)

This PR addresses two existing issues with some data tables that are causing inconsistencies between the URL querystring parameters and the state of the filters panel and taglist buttons.(active filters). Below is each issue and its fix.

  1. Issue: Certain checkbox filters (in a checkbox dropdown widget) are not added to the URL querystring parameters when chosen, or do not remove from the querystring when manually removed via the filter panel or taglist buttons.

    Fix: Use the correct macro variable in committee_types.jinja for data-name attributes of fieldsets to ensure the Jinja tag resolves to the correct values for the specific datatable it is on. There should be at least one fieldset on the page for each of the three data-name variables. (designation, committee_type, and organization_type)

  2. Issue: Having checkboxes with differing name attributes inside a fieldset of a certain data-name confuses existing logic that compares all filters in the filter panel against the filters indicated by querystring parameters. It expects a fieldsets data-name and all of its childrens' name attributes to be the same.

    • This causes some filters to be stripped from the URL and sometimes not be included in the filtered API call for data.
    • Another symptom of this is that if there are two checkboxes with the same letter value, having one in a querystring can sometimes automatically activate (or replace) the other.

    Fix: Upon page load, programmatically activate all checkbox filters indicated in the querystring parameters (if they are not already activated by existing logic).
    Also add three hidden empty fieldsets to committee-types.jinja with data-names of the three needed variables: (( designation }}, {{ committee_type }} and {{ organization_type }}. Then name the other fieldsets with a descriptive data-name for grouping purposes, but not with one of the variable names or a Jinja tag that resolves to one of the variables.

This will ensure that the URL matches the state of the filter panel and vice versa allowing datatable URLs to remain accurate when shared, bookmarked or refreshed.

Required reviewers

One frontend, one backend. Optional: one content/UX

Impacted areas of the application

These fixes are specifically for the committee-type filters where we have checkbox filters who's name attribute is different than its parent fieldset's data-name attribute. The changes allow us to maintain this unique structure without confusing the existing logic that expects them to match.

modified: data/templates/macros/filters/committee-types.jinja
modified: fec/static/js/modules/tables.js

Related PR

#5078

How to test

This branch is deployed to feature space, see pages below listed under "linked to feature"

Copy link
Contributor

@pkfec pkfec left a comment

Choose a reason for hiding this comment

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

Excellent work @johnnyporkchops. I have tested your changes on feature space. Unlike in production, the filters are rendering on the URL and accurate counts/results are showing on the datatables.

Copy link
Member

@cnlucas cnlucas left a comment

Choose a reason for hiding this comment

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

Went through everything on feature and it worked well. Thanks John, this is impressive!

Copy link
Member

@patphongs patphongs left a comment

Choose a reason for hiding this comment

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

@johnnyporkchops Great work on this, it all works as expected 🎉 . I also appreciated all the comments you added to this, I only had very minor things to fix up.

tasks.py Outdated
@@ -76,7 +76,7 @@ def _detect_space(repo, branch=None, yes=False):
('stage', lambda _, branch: branch.startswith('release')),
('dev', lambda _, branch: branch == 'develop'),
# Uncomment below and adjust branch name to deploy desired feature branch to the feature space
# ('feature', lambda _, branch: branch == '[BRANCH NAME]'),
('feature', lambda _, branch: branch == 'feature/4010-shareable-datatable-links'),
Copy link
Member

Choose a reason for hiding this comment

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

Comment out deploy to feature

}
}, 0);

// No Set-timrouot needed on pages without two filter panels...
Copy link
Member

Choose a reason for hiding this comment

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

"Set-timrouot" spelling needs correction

}
});

// Put 0-secopnd, set-timeout on receipts/disbursements datatales so checkoxes are availale to check...
Copy link
Member

Choose a reason for hiding this comment

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

"0-secopnd" spelling needs correction. "datatales" spelling needs correction.

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2022

Codecov Report

Merging #5098 (1fecc6d) into develop (d6481cb) will decrease coverage by 0.60%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5098      +/-   ##
===========================================
- Coverage    75.54%   74.93%   -0.61%     
===========================================
  Files          125      125              
  Lines         8050     8118      +68     
  Branches       637      648      +11     
===========================================
+ Hits          6081     6083       +2     
- Misses        1969     2035      +66     
Impacted Files Coverage Δ
fec/fec/static/js/modules/tables.js 48.16% <0.00%> (-5.92%) ⬇️
fec/fec/static/js/modules/calendar.js 92.08% <0.00%> (-0.72%) ⬇️

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 d6481cb...1fecc6d. Read the comment docs.

Copy link
Member

@patphongs patphongs left a comment

Choose a reason for hiding this comment

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

👍 Good to go, thanks @johnnyporkchops !

@patphongs patphongs merged commit fc95ab1 into develop Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to share accurate datatable links
5 participants