scm-spain / boros-CMP

GDPR CMP (Consent Managment Provider) implementation
MIT License
5 stars 1 forks source link

Hotfix/http resources downloaded twice #73

Closed alextremp closed 4 years ago

alextremp commented 4 years ago

Description

As validated in the first commit: https://github.com/scm-spain/boros-CMP/commit/4192a37c5c953b3f9d5971bebdec6b3225257671

The chained vendor list repository was not working as expected: instead of reading from the inmemory repo and delegating to the http one only when the inmemory data was not existing, in the case of N simultaneous requests, as it's resolved in async mode, the chained repo was delegating part of the N promises to the http repo, so it was doing N HTTP requests instead of 1 for the same global vendor list JSON resource.

image

Solves ticket/s

Expected behavior

When a vendor list is requested:

Review steps

Memetized description

mandatory

codecov-io commented 4 years ago

Codecov Report

Merging #73 into master will decrease coverage by 0.02%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage   94.14%   94.11%   -0.03%     
==========================================
  Files          73       73              
  Lines        2271     2278       +7     
  Branches      155      156       +1     
==========================================
+ Hits         2138     2144       +6     
- Misses        133      134       +1
Impacted Files Coverage Δ
...tructure/repository/ChainedVendorListRepository.js 100% <100%> (ø) :arrow_up:
src/cmp/domain/vendor_consents/VendorConsents.js 86.36% <0%> (-4.34%) :arrow_down:
...epository/ConsentStringVendorConsentsRepository.js 98.9% <0%> (+1.05%) :arrow_up:

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 7798b5a...0819de0. Read the comment docs.

Osgar commented 4 years ago

@alextremp node_js 12?

alextremp commented 4 years ago

yes, @Osgar, with the version set to 8, the Promise.allSettled does not exist and the tests fail