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

[DT-777] Filter by participant count #2733

Merged
merged 23 commits into from
Dec 2, 2024

Conversation

rjohanek
Copy link
Contributor

@rjohanek rjohanek commented Nov 21, 2024

Addresses

Jira Ticket: https://broadworkbench.atlassian.net/browse/DT-777

Summary

Adds a filter for participant size

DatasetSearchTable:

  • generalize the filter handler to work for non-Array filters
  • generalize the numFiltersSelected method to work for non-Array filters
  • update assembleFullQuery to include range chunk
  • rename isFiltered to isFilteredArray because it is only used for array filters
  • remove unused arguments to filterHandler and unused prop to DatasetFilterList

DatasetFilterList

  • add new filter range constant
  • add new filter to the filter list

Testing

Unit tests
Note: cy.clock() is needed to mock the clock for debounced calls

Visual

Screen.Recording.2024-11-21.at.9.59.40.AM.mov

Have you read Terra's Contributing Guide lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@rjohanek rjohanek requested a review from a team as a code owner November 21, 2024 14:57
@rjohanek rjohanek requested review from pshapiro4broad and snf2ye and removed request for a team November 21, 2024 14:57
Copy link
Member

@pshapiro4broad pshapiro4broad left a comment

Choose a reason for hiding this comment

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

Looks OK, just a few misc comments.

src/components/data_search/DatasetSearchTable.jsx Outdated Show resolved Hide resolved
Comment on lines 61 to 62
} else {
if (filters[category]) {
Copy link
Member

Choose a reason for hiding this comment

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

I would combine an if after an else here

Suggested change
} else {
if (filters[category]) {
} else if (filters[category]) {

src/components/data_search/DatasetFilterList.jsx Outdated Show resolved Hide resolved
src/components/data_search/DatasetSearchTable.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

I like most of @pshapiro4broad 's suggestions! The only one I'm not sure about is the filterHandler refactor.

import DatasetFilterList from '../../../src/components/data_search/DatasetFilterList';

const duosUser = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused

@rjohanek rjohanek requested a review from rushtong November 22, 2024 16:45
Copy link
Member

@pshapiro4broad pshapiro4broad left a comment

Choose a reason for hiding this comment

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

Looks good, glad to see that the flaky tests have been fixed

Comment on lines 56 to 57
const anyFiltersSelected = (filters) => {
return Object.values(filters).some((filter) => {
Copy link
Member

Choose a reason for hiding this comment

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

For a single line lambda, you can omit the { return }

Suggested change
const anyFiltersSelected = (filters) => {
return Object.values(filters).some((filter) => {
const anyFiltersSelected = (filters) =>
Object.values(filters).some((filter) => {

I know in terra-ui this was the standard, not sure if duos-ui has an opinion about this.

src/components/data_search/DatasetSearchTable.jsx Outdated Show resolved Hide resolved
cy.intercept(
{method: 'POST', url: '**/search/index'}, (req) => {
return handler(req, '{"range":{"participantCount":{"gte":null,"lte":50}}}');
}).as('searchIndex1');
Copy link
Member

Choose a reason for hiding this comment

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

Is it required that searchIndex1 must be different than searchIndex2 in the other test? I would expect the alias names to be independent across tests so the same name could be used for both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question, no I don't think so I just changed them to detangle as part of the debugging effort but I can change them back now

cy.wait('@searchIndex').then((response) => {
expect(response.response.body[0]).to.equal('filtered');
});
cy.get('@searchIndex.all').should('have.length', 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that this test does pass, but I'm confused as to what the .all part of this alias selector is doing and where it is defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I found why it is undocumented:

Since .all is experimental, I would recommend looking into a different approach to this specific case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's another approach, less concise, but uses documented cypress features:

      let count = 0;
      cy.wait('@searchIndex').then((response) => {
        expect(response.response.body[0]).to.equal('filtered');
        count++;
      });
      cy.get('@searchIndex').then(() => {
        expect(count).to.equal(1);
      });

}).as('searchIndex');
mount(<DatasetSearchTable {...props} />);
// first clear the default value (100), without clearing first, type('50') would result in input of 10050
cy.get('#participantCountMax-range-input').clear().type('50');
Copy link
Contributor

Choose a reason for hiding this comment

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

Cypress recommends not chaining calls after clear. It's a little more verbose, but safer to:

      const range = cy.get('#participantCountMax-range-input');
      range.clear();
      range.type('50');

mount(<DatasetSearchTable {...props} />);
// first clear the default value (100), without clearing first, type('50') would result in input of 10050
cy.get('#participantCountMax-range-input').clear().type('50');
cy.tick(150);
Copy link
Contributor

Choose a reason for hiding this comment

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

Cypress recommends that there be a clock command that precedes the tick command: https://docs.cypress.io/api/commands/tick

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use the clock command in the before each method, but I could move it into each individual test

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, I did miss that. No need if it's there, that should cover it.

Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

See comments inline, looks good, thank you 👍🏽

@rjohanek rjohanek merged commit 6b78e70 into develop Dec 2, 2024
9 checks passed
@rjohanek rjohanek deleted the rj/dt-777-filter-participant-count branch December 2, 2024 15:33
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.

5 participants