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

refactor(dateHelper): swatch-260 remove moment.js from dateHelper #1220

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

vbusch
Copy link
Contributor

@vbusch vbusch commented Oct 25, 2023

What's included

sw-260 Replace moment.js

Notes

Removed moment.js from the date helper. This is a non functional change. There should be no noticeable UI differences.
TODO: remove it from the other files.

How to test

Coverage and basic unit test check

  1. update the NPM packages with $ yarn
  2. $ yarn test
  3. confirm tests come back clean

Proxy run check

  1. update the NPM packages with $ yarn
  2. make sure Docker is running, plus on network, then
  3. $ yarn start:proxy
  4. http://localhost:3000/apps/subscriptions/rhel
  5. change the time range for the cpu socket usage. And verify the network request has the correct range.

Check the build

  1. update the NPM packages with $ yarn
  2. $ yarn build
  3. confirm tests come back clean

Updates issue/story

sw-260

@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2023

Codecov Report

Merging #1220 (00ab2ba) into main (d46fab0) will increase coverage by 0.04%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1220      +/-   ##
==========================================
+ Coverage   92.16%   92.21%   +0.04%     
==========================================
  Files         122      122              
  Lines        4251     4264      +13     
  Branches     1786     1790       +4     
==========================================
+ Hits         3918     3932      +14     
+ Misses        314      312       -2     
- Partials       19       20       +1     
Files Coverage Δ
src/common/dateHelpers.js 93.05% <91.66%> (+3.22%) ⬆️

Continue to review full report in Codecov by Sentry.

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

@vbusch vbusch changed the title swatch-260 remove moment.js from dateHelper refactor(dateHelper): swatch-260 remove moment.js from dateHelper Oct 25, 2023
@mirekdlugosz
Copy link
Collaborator

@vbusch if you have access to iqe-curiosity-plugin on internal GitLab, please take a look at iqe_curiosity/tests/unittests_utils.py. There's a number of unit tests that cover cases that I have seen failing over the years. I think it might be good idea to add at least some of them to unit tests here, to ensure this change is backwards compatible.

@diegomaranhao can help you with accessing these tests, and is up to date with whether they are still running or not.

@cdcabrera cdcabrera self-requested a review October 26, 2023 15:22
@cdcabrera cdcabrera added tech debt We've ignored it this long... 202312 project phase labels Oct 26, 2023
Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

All minor stuff, mostly copy. Looks good at the functional level!

src/common/dateHelpers.js Outdated Show resolved Hide resolved
src/common/dateHelpers.js Show resolved Hide resolved
src/common/dateHelpers.js Outdated Show resolved Hide resolved
src/common/dateHelpers.js Outdated Show resolved Hide resolved
src/common/dateHelpers.js Outdated Show resolved Hide resolved
src/common/dateHelpers.js Outdated Show resolved Hide resolved
src/common/__tests__/dateHelpers.test.js Outdated Show resolved Hide resolved
src/common/__tests__/dateHelpers.test.js Outdated Show resolved Hide resolved
Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

minor parameter adjustment

src/common/dateHelpers.js Outdated Show resolved Hide resolved
Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

lgtm!

stepped through the combinations against data in staging, ranges appear to maintain the original ranges for standard granularity (daily, weekly, monthly, quarterly), and the additional "ranged granularity" we use for on-demand displays

@vbusch this is an approval without being a github approval, reason, before we merge this update, reviewing a sync between envs for variant work

Signed-off-by: Vanessa Busch <vbusch@redhat.com>
@cdcabrera cdcabrera merged commit b46721e into RedHatInsights:main Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
202312 project phase tech debt We've ignored it this long...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants