Do not retain values when switching between rules in the DateFilter component #7423

aprea · 2021-07-27T05:27:12Z

Also fixes Automattic/woocommerce-payments#1632

The DateFilter component used in the AdvancedFilters component incorrectly retains values when switching between "Before/After" and "Between" rules.

This creates a few problems (see the steps to reproduce below for the bugs / undesired behavior).

This PR changes this behavior to ensure:

Values are retained when switching between "Before" & "After" rules.
Values are reset when switching to/from the "Between" rule.

Testing instructions

Storybook

Run npm run storybook
Visit http://localhost:6007/ in your browser
Navigate to the AdvancedFilters story from the left-hand side panel
Click the "Open canvas in new tab" button in the top right
Add a "Before" date filter.
Modify the date value to the 1st of the current month.
Switch the rule to "Between"
Observe the previously set date value was reset and the between date fields are empty.
Change the value of the first date field to the 1st of the current month and the value of the second date field to the last day of the current month.
Click the filter button.
Observe the query params in the address bar are correctly updated to reflect this change.
Switch the rule back to "Before"
Observe the previously set date values were reset and the new "Before" date field is empty.
Modify the date value to the 1st of the current month.
Click the filter button.
Observe the query params in the address bar are correctly updated to reflect this change.

WCPay

In woocommerce-admin:

Ensure you're on the fix/7422-date-filter-retains-values branch.
Run npm run build
Run ./bin/make-zip.sh woocommerce-admin.zip
Copy woocommerce-admin.zip

In woocommerce-payments

Unzip woocommerce-admin.zip into the docker/wordpress/wp-content/plugins directory
Navigate to /wp-admin/plugins.php and activate the WooCommerce Admin plugin.
Navigate to /wp-admin/admin.php?page=wc-admin&path=%2Fpayments%2Ftransactions and attempt the steps to reproduce as detailed in [GlobalStep] User navigates to the Blank screen upon filtering transactions. Automattic/woocommerce-payments#1632

To reproduce the bugs

Temporarily apply the changes in packages/components/src/advanced-filters/stories/index.js from this branch to the main branch for testing.

In Storybook:

Add a "Before" date filter to the AdvancedFilters component.
Modify the date value to the 1st of the current month.
Switch the rule to "Between"
Bug: Observe the previously set date value now appears in the second date field.
Change the value of the first date field to the 1st of the current month and the value of the second date field to the last day of the current month.
Click the filter button.
Observe the query params in the address bar are correctly updated to reflect this change.
Switch the rule back to "Before"
Bug: Observe the previously set date value in the second "Between" field now appears as the before date field value.
Click the filter button.
Bug: Observe the query params in the address bar now appear as follows (URL decoded)

date_before[0]=2021-07-01&date_before[1]=2021-07-27

…omponent

packages/components/src/advanced-filters/date-filter.js

samueljseay · 2021-07-30T03:23:47Z

@aprea Thanks for this!

A quick look at this and it looks good to me, but I'll need to test our existing usages of it just to make sure it doesn't unintentionally break anything.

I know it's a pain but could you add a changelog entry into the packages changelog? We don't have a consolidated way to handle package and non-package changelogs right now, so you can just add the entry under Unreleased in packages/components/CHANGELOG.md. And you can remove the entry you added via changelogger (sorry I know the instructions are confusing right now, we could improve this).

aprea · 2021-07-30T04:11:54Z

Changelog entry fixed (I think), thanks for the heads up @samueljseay

kalessil

Did testing following the instructions and it works as expected.
We need approval from the maintenance team though.

kalessil · 2021-07-30T09:15:12Z

@samueljseay : adding you as a reviewer so we are getting approval from maintainers before moving forward.

naman03malhotra

The changes look good to me, and work as expected 🥳, lift a comment wrt to unit tests.

However, I can't reproduce this issue: Automattic/woocommerce-payments#1632 with storybook (on main branch), can you please test this with wcpay code once and see if this is fixed.

I've tested by copying the build files from node_modules of wcpay_admin to wcpay node_modules, and I can still see this issue. Maybe I am missing something? 🤔

ps: somewhere the setState is being called recursively. Also, one of the checks is failing.

naman03malhotra · 2021-07-30T16:31:09Z

packages/components/src/advanced-filters/date-filter.js

+		const { onFilterChange } = this.props;
+		const { rule } = this.state;
+
+		let newDateState = null;
+		let shouldResetValue = false;
+
+		if ( [ rule, newRule ].includes( 'between' ) ) {
+			newDateState = {
+				before: null,
+				beforeText: '',
+				beforeError: null,
+				after: null,
+				afterText: '',
+				afterError: null,
+			};
+
+			shouldResetValue = true;
+		}
+
+		this.setState( {
+			rule: newRule,
+			...newDateState,
+		} );
+
+		onFilterChange( 'rule', newRule, shouldResetValue );


Can we add a unit test for this change, something like:
should reset value/state when the rule is switched

Tests added in b8a1ff9

aprea · 2021-08-02T01:15:25Z

The changes look good to me, and work as expected 🥳, lift a comment wrt to unit tests.

Woo, thanks for the review 😄

However, I can't reproduce this issue: Automattic/woocommerce-payments#1632 with storybook (on main branch), can you please test this with wcpay code once and see if this is fixed.

I've tested by copying the build files from node_modules of wcpay_admin to wcpay node_modules, and I can still see this issue. Maybe I am missing something? 🤔

I don't think this will work because in the wcpay local environment the woocommerce-admin dist files are being enqueued from the woocommerce plugin rather than the woocommerce-payments plugin.

I also tried using npm link but had no luck.

You can perform some testing with the following steps:

In woocommerce-admin:

Ensure you're on the fix/7422-date-filter-retains-values branch.
Run npm run build
Run ./bin/make-zip.sh woocommerce-admin.zip
Copy woocommerce-admin.zip

In woocommerce-payments

Unzip woocommerce-admin.zip into the docker/wordpress/wp-content/plugins directory
Navigate to /wp-admin/plugins.php and activate the WooCommerce Admin plugin.
Navigate to /wp-admin/admin.php?page=wc-admin&path=%2Fpayments%2Ftransactions and attempt the steps to reproduce as detailed in [GlobalStep] User navigates to the Blank screen upon filtering transactions. Automattic/woocommerce-payments#1632

ps: somewhere the setState is being called recursively.

Thanks, I'll take a look

Also, one of the checks is failing.

This is because of the changelog, I'm a little confused about this process, I'll wait for one of the maintainers of this repo to tell me what I've done wrong here 😄

joshuatf · 2021-08-03T02:02:00Z

This is because of the changelog, I'm a little confused about this process, I'll wait for one of the maintainers of this repo to tell me what I've done wrong here 😄

I'll try and give this a formal review tomorrow, but no need to worry about the failing test as this has not been updated to our new flow. We've recently started using changelogger to add entries, so you can run npm run changelogger -- add to add your changelog entry (just note that this will not fix that failing test 😄 ).

Edit: We're only using changelogger for the app changelog AFAIK so your package changelog entry looks correct.

kalessil · 2021-08-05T07:21:42Z

Hey @joshuatf 👋, thanks for stepping into the review ! Any chance to review the PR today, so we could wrap it up this week?

joshuatf · 2021-08-05T12:42:59Z

@kalessil Sorry, my schedule has become fairly packed with release issues so I won't be able to review this week.

louwie17

This tested well, and code overall looks good, I did leave one suggestion. This would require a potential bigger refactor, so I am fine if this happens in a separate PR and we can merge this as is (unless you feel inclined to make the changes as part of this PR).

To make sure the changelog action passes, you can add No changelog necessary as it is in the component package. The Github action looks for the no changelog in the description and passes if that is present (might have to re-run the task after you updated the description).

Thanks for updating and fixing this @aprea 🎉

louwie17 · 2021-08-05T19:20:33Z

packages/components/src/advanced-filters/test/advanced-filters.test.js

+		// The previous value should be reset when switching to/from the "Between" rule.
+		expect( dateFields[ 0 ] ).not.toHaveValue();
+		expect( dateFields[ 1 ] ).not.toHaveValue();
+	} );


Nice work on these tests 🎉

louwie17 · 2021-08-05T19:35:37Z

packages/components/src/advanced-filters/index.js

 		const newActiveFilters = [ ...this.state.activeFilters ];
 		newActiveFilters[ index ] = {
 			...newActiveFilters[ index ],
 			[ property ]: value,
+			...( shouldResetValue === true ? { value: null } : {} ),


Hmm I am not a huge fan of adding this extra parameter, the onFilterChange functions seems to only be used for updating the value, so adding a shouldResetValue for the one case where we don't update the value seems confusing.
I would be good with going to different routes for this:

Add a separate onRuleChange function that handles the reset value, although this is very specific (so maybe not a great option)

Update the onFilterChange function to pass in a updated filterValue object instead:

onFilterChange( index, updatedFilterValue ) { const newActiveFilters = [ ...this.state.activeFilters ]; newActiveFilters[ index ] = { ...newActiveFilters[ index ], ...updatedFilterValue } this.setState( { activeFilters: newActiveFilters } ); }

This way it is more explicit in the date-filter what you are doing ( updating the role and value )

Hey @louwie17, thanks for the review!

Hmm I am not a huge fan of adding this extra parameter

I agree. I went with this approach as it was low touch and didn't require too much refactoring.

Update the onFilterChange function to pass in a updated filterValue object ... This way it is more explicit in the date-filter what you are doing ( updating the role and value )

This is definitely a better approach but would require refactoring the existing filter components (e.g. AttributeFilter, NumberFilter, SearchFilter, etc.)

Ultimately, this probably isn't too difficult but given this is my first PR to this repo I didn't want to get too adventurist.

This would require a potential bigger refactor, so I am fine if this happens in a separate PR and we can merge this as is (unless you feel inclined to make the changes as part of this PR).

Cool thanks. In that case, I might go ahead with the current approach and circle back with the aim of improving this in the future.

GitHub issue to refactor this with the proposed approach #7470

…omponent (woocommerce/woocommerce-admin#7423) * Do not retain values when switching between rules in the DateFilter component * Add changelog entry * Remove unnecessary use of the ternary operator * Fix the changelog entry * Add tests * Add changelog entry Co-authored-by: Chris <chris.aprea@automattic.comchris.aprea@automattic.comchris.aprea@automattic.com>

Do not retain values when switching between rules in the DateFilter c…

2dfecad

…omponent

aprea added the status: needs review label Jul 27, 2021

aprea requested review from dmvrtx, kalessil, RadoslavGeorgiev, htdat, naman03malhotra, jessy-p and shendy-a8c July 27, 2021 05:27

aprea self-assigned this Jul 27, 2021

probot-autolabeler bot added @woocommerce/components Packages labels Jul 27, 2021

github-actions bot added the needs changelog entry label Jul 27, 2021

Add changelog entry

57ac8fd

github-actions bot removed the needs changelog entry label Jul 27, 2021

aprea mentioned this pull request Jul 27, 2021

[GlobalStep] User navigates to the Blank screen upon filtering transactions. Automattic/woocommerce-payments#1632

Closed

3 tasks

aprea commented Jul 27, 2021

View reviewed changes

packages/components/src/advanced-filters/date-filter.js Outdated Show resolved Hide resolved

Chris added 2 commits July 28, 2021 09:28

Remove unnecessary use of the ternary operator

7855458

Merge branch 'main' into fix/7422-date-filter-retains-values

a4784f0

samueljseay added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed status: needs review labels Jul 30, 2021

Fix the changelog entry

8056c8f

github-actions bot added the needs changelog entry label Jul 30, 2021

aprea added status: needs review and removed needs changelog entry needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Jul 30, 2021

kalessil approved these changes Jul 30, 2021

View reviewed changes

kalessil requested a review from samueljseay July 30, 2021 09:13

kalessil removed request for dmvrtx, RadoslavGeorgiev, htdat, jessy-p and shendy-a8c July 30, 2021 09:16

naman03malhotra reviewed Jul 30, 2021

View reviewed changes

github-actions bot added the needs changelog entry label Aug 2, 2021

Add tests

b8a1ff9

probot-autolabeler bot added the tool: monorepo infrastructure label Aug 2, 2021

louwie17 approved these changes Aug 5, 2021

View reviewed changes

louwie17 added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. status: ready to merge and removed status: needs review labels Aug 5, 2021

aprea mentioned this pull request Aug 6, 2021

Refactor the onFilterChange method in the AdvancedFilters component #7470

Closed

aprea force-pushed the fix/7422-date-filter-retains-values branch from e4edfe7 to f9c2420 Compare August 6, 2021 05:21

Add changelog entry

a91888b

aprea force-pushed the fix/7422-date-filter-retains-values branch from f9c2420 to a91888b Compare August 6, 2021 05:24

github-actions bot removed the needs changelog entry label Aug 6, 2021

aprea merged commit 0c83dcf into main Aug 6, 2021

aprea deleted the fix/7422-date-filter-retains-values branch August 6, 2021 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not retain values when switching between rules in the DateFilter component #7423

Do not retain values when switching between rules in the DateFilter component #7423

aprea commented Jul 27, 2021 •

edited

Loading

samueljseay commented Jul 30, 2021 •

edited

Loading

aprea commented Jul 30, 2021 •

edited

Loading

kalessil left a comment

kalessil commented Jul 30, 2021 •

edited

Loading

naman03malhotra left a comment

naman03malhotra Jul 30, 2021

aprea Aug 2, 2021

aprea commented Aug 2, 2021 •

edited

Loading

joshuatf commented Aug 3, 2021 •

edited

Loading

kalessil commented Aug 5, 2021

joshuatf commented Aug 5, 2021

louwie17 left a comment

louwie17 Aug 5, 2021

louwie17 Aug 5, 2021

aprea Aug 6, 2021 •

edited

Loading

Do not retain values when switching between rules in the DateFilter component #7423

Do not retain values when switching between rules in the DateFilter component #7423

Conversation

aprea commented Jul 27, 2021 • edited Loading

Testing instructions

To reproduce the bugs

samueljseay commented Jul 30, 2021 • edited Loading

aprea commented Jul 30, 2021 • edited Loading

kalessil left a comment

Choose a reason for hiding this comment

kalessil commented Jul 30, 2021 • edited Loading

naman03malhotra left a comment

Choose a reason for hiding this comment

naman03malhotra Jul 30, 2021

Choose a reason for hiding this comment

aprea Aug 2, 2021

Choose a reason for hiding this comment

aprea commented Aug 2, 2021 • edited Loading

joshuatf commented Aug 3, 2021 • edited Loading

kalessil commented Aug 5, 2021

joshuatf commented Aug 5, 2021

louwie17 left a comment

Choose a reason for hiding this comment

louwie17 Aug 5, 2021

Choose a reason for hiding this comment

louwie17 Aug 5, 2021

Choose a reason for hiding this comment

aprea Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

aprea commented Jul 27, 2021 •

edited

Loading

samueljseay commented Jul 30, 2021 •

edited

Loading

aprea commented Jul 30, 2021 •

edited

Loading

kalessil commented Jul 30, 2021 •

edited

Loading

aprea commented Aug 2, 2021 •

edited

Loading

joshuatf commented Aug 3, 2021 •

edited

Loading

aprea Aug 6, 2021 •

edited

Loading