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

Sc 30945/client dmi partners inc pulled an export #130

Conversation

richardfordjunior
Copy link

Description of the change

This fixes the issue where rich-types are not being properly masked in exports

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Configuration change
  • Technical Debt
  • Documentation

Related tickets

https://app.shortcut.com/active-prospect/story/30945/client-dmi-partners-inc-pulled-an-export-of-tf-data-service-info-for-leads-even-though-the-certs-are-not-masked-many-of-the

Checklists

Development and Testing

  • Lint rules pass locally.
  • The code changed/added as part of this pull request has been covered with tests, or this PR does not alter production code.
  • All tests related to the changed code pass in development, or tests are not applicable.

Code Review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached.
  • At least two engineers have been added as "Reviewers" on the pull request.
  • Changes have been reviewed by at least two other engineers who did not write the code.
  • This branch has been rebased off master to be current.

Tracking

  • Issue from Clubhouse has a link to this pull request.
  • This PR has a link to the issue in Clubhouse.

QA

  • This branch has been deployed to staging and tested.

@richardfordjunior richardfordjunior temporarily deployed to test November 8, 2021 14:51 Inactive
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2021

Codecov Report

Merging #130 (ff75e61) into master (d2da639) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
+ Coverage   97.04%   97.09%   +0.05%     
==========================================
  Files          34       34              
  Lines         576      586      +10     
==========================================
+ Hits          559      569      +10     
  Misses         17       17              
Impacted Files Coverage Δ
lib/index.js 87.62% <100.00%> (+1.42%) ⬆️

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 a97448d...ff75e61. Read the comment docs.

Copy link
Contributor

@mikebetts mikebetts left a comment

Choose a reason for hiding this comment

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

Looks like this covers masking rich types in general, but it seems like this still isn't handling the case where the masked property itself is a rich type. It is supposed to look at its boolean value to determine whether to mask or not.

lib/index.js Outdated
@@ -55,9 +55,15 @@ const mask = function (obj, doMask = false) {
for (const key in obj) {
const value = obj[key];
if (key === 'valid') { continue; }
obj[key] = mask(value, (obj.masked != null) || doMask);
obj[key] = mask(value, (obj.masked != null || obj.masked !== undefined) || doMask);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by this - if obj.masked is an object, won't it always return true and therefore mask despite what the masked value is set to in that object?

Copy link
Author

Choose a reason for hiding this comment

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

I see what you mean. I'll update to cover when the obj.masked is a rich type.

Copy link
Author

Choose a reason for hiding this comment

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

Yes.

@richardfordjunior richardfordjunior temporarily deployed to test November 9, 2021 22:19 Inactive
@richardfordjunior richardfordjunior temporarily deployed to test November 9, 2021 22:19 Inactive
@richardfordjunior richardfordjunior temporarily deployed to test November 9, 2021 22:19 Inactive
@richardfordjunior richardfordjunior temporarily deployed to test November 10, 2021 00:17 Inactive
@richardfordjunior richardfordjunior temporarily deployed to test November 10, 2021 00:17 Inactive
@richardfordjunior richardfordjunior temporarily deployed to test November 10, 2021 00:17 Inactive
@richardfordjunior richardfordjunior temporarily deployed to test November 12, 2021 18:55 Inactive
@richardfordjunior richardfordjunior temporarily deployed to test November 12, 2021 18:56 Inactive
@richardfordjunior richardfordjunior temporarily deployed to test November 12, 2021 18:56 Inactive
@mikebetts mikebetts temporarily deployed to test November 17, 2021 21:40 Inactive
@mikebetts mikebetts temporarily deployed to test November 17, 2021 21:40 Inactive
@mikebetts mikebetts temporarily deployed to test November 17, 2021 21:40 Inactive
@richardfordjunior richardfordjunior temporarily deployed to test November 19, 2021 15:18 Inactive
@richardfordjunior richardfordjunior temporarily deployed to test November 19, 2021 15:18 Inactive
@richardfordjunior richardfordjunior temporarily deployed to test November 19, 2021 15:18 Inactive
Copy link
Contributor

@asedarski asedarski left a comment

Choose a reason for hiding this comment

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

Provided the assertions in the tests are correct (I don't understand the business logic behind what's masked and what isn't), LGTM

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.

4 participants