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

Eliminate the duplicate functions used to group shipping time data #2726

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

eason9487
Copy link
Member

Changes proposed in this Pull Request:

Closes #1426

This PR eliminates the duplicate functions used to group shipping time data.

  • Move getCountriesTimeArray to the utils directory and rename to getShippingTimesGroups.
  • Replace the getShippingTimesGroups function in the useSaveShippingTimes hook with the one in utils.
  • Internalize the getShippingTimeMapKey util into getShippingTimesGroups util.
  • Extra change: Remove an unreferenced AggregatedShippingTime type.

Detailed test instructions:

  1. Go to step 2 of the onboarding flow.
  2. Check if the shipping times can be saved correctly.
  3. Go to the free listings editing page.
  4. Check if the shipping times can be saved correctly.

Changelog entry

Dev - Eliminate the duplicate functions used to group shipping time data.

@eason9487 eason9487 requested a review from a team December 13, 2024 09:28
@eason9487 eason9487 self-assigned this Dec 13, 2024
@github-actions github-actions bot added the changelog: dev Developer-facing only change. label Dec 13, 2024
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 63.5%. Comparing base (63ca753) to head (c9a7d61).

Files with missing lines Patch % Lines
...ngs/shipping-time-setup/shipping-countries-form.js 0.0% 1 Missing ⚠️
js/src/data/actions.js 0.0% 1 Missing ⚠️
js/src/utils/getShippingTimesGroups.js 50.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                      Coverage Diff                      @@
##           fix/empty-max-shipping-time   #2726     +/-   ##
=============================================================
+ Coverage                         63.4%   63.5%   +0.1%     
=============================================================
  Files                              338     337      -1     
  Lines                             5203    5193     -10     
  Branches                          1274    1273      -1     
=============================================================
- Hits                              3301    3299      -2     
+ Misses                            1728    1721      -7     
+ Partials                           174     173      -1     
Flag Coverage Δ
js-unit-tests 63.5% <50.0%> (+0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
js/src/data/reducer.js 82.5% <100.0%> (ø)
js/src/hooks/useSaveShippingTimes.js 31.6% <ø> (+5.7%) ⬆️
...ngs/shipping-time-setup/shipping-countries-form.js 0.0% <0.0%> (ø)
js/src/data/actions.js 15.8% <0.0%> (ø)
js/src/utils/getShippingTimesGroups.js 11.1% <50.0%> (ø)

Copy link
Member

@ianlin ianlin left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up the duplicate functions. Confirmed saving shipping times works normally after those changes.

Base automatically changed from fix/empty-max-shipping-time to develop December 16, 2024 07:55
@eason9487 eason9487 merged commit af019af into develop Dec 16, 2024
9 checks passed
@eason9487 eason9487 deleted the dev/1426-eliminate-duplicate-functions branch December 16, 2024 08:04
@eason9487 eason9487 mentioned this pull request Dec 18, 2024
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: dev Developer-facing only change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code refactor: aggregate shipping time into groups (getCountriesTimeArray and getShippingTimesGroups)
2 participants