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

Implement Circuit Breaker for fetching blog news #14586

Merged
merged 15 commits into from Aug 6, 2019

Conversation

@Progi1984
Copy link
Contributor

commented Jul 11, 2019

Questions Answers
Branch? develop
Description? Implement Circuit Breaker for fetching blog news
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #12827
How to test? * Check if news are well displayed in backoffice
* Activate plane mode (no wifi, no network)
* Check if news are well displayed in backoffice (cache is used)

This change is Reviewable

@Progi1984 Progi1984 requested review from jolelievre and PrestaShop/prestashop-core-developers Jul 11, 2019
@colinegin colinegin added this to To be reviewed in PrestaShop 1.7.7 Jul 11, 2019
classes/NewsFetcher.php Outdated Show resolved Hide resolved
classes/NewsFetcher.php Outdated Show resolved Hide resolved
Copy link
Contributor

left a comment

Hi @Progi1984,
nice job but as we discussed I think you can make this first circuit breaker integration a bit sexier using Symfony and defining basic services that will be reused in other use cases.

classes/NewsFetcher.php Outdated Show resolved Hide resolved
classes/NewsFetcher.php Outdated Show resolved Hide resolved
@colinegin colinegin moved this from To be reviewed to In progress in PrestaShop 1.7.7 Jul 11, 2019
Copy link
Contributor

left a comment

This is getting better! Noce job
A few feedback to address again ^^

And I think you can try adding unit tests for NewsProvider (you just need to mock the circuit breaker, that's the advantage of injecting it this way)

@marionf marionf removed this from In progress in PrestaShop 1.7.7 Jul 12, 2019
Copy link
Contributor

left a comment

Nice job @Progi1984
Some little feedback left, but we're almost there!

src/Adapter/News/NewsDataProvider.php Outdated Show resolved Hide resolved
src/Adapter/News/NewsDataProvider.php Outdated Show resolved Hide resolved
src/Adapter/News/NewsDataProvider.php Show resolved Hide resolved
src/Adapter/News/NewsDataProvider.php Outdated Show resolved Hide resolved
src/Adapter/News/NewsDataProvider.php Outdated Show resolved Hide resolved
Copy link
Contributor

left a comment

Haha one last change :D

src/Adapter/News/NewsDataProvider.php Show resolved Hide resolved
@Progi1984 Progi1984 requested a review from jolelievre Jul 18, 2019
@jolelievre

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

@Matt75 I think this will go into the core for now, so that we have the basic circuit breaker services available. But about the specific management of news, this could be migrated to mbo later I guess.

@sarahdib

This comment has been minimized.

Copy link

commented Jul 24, 2019

@Progi1984 when the wifi is disable the block is display but not like when I have wifi.

image

@Progi1984

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@sarahdib Sorry, I have bad read a specification. It's corrected : the block must be displayed with no news inside.

@sarahdib

This comment has been minimized.

Copy link

commented Jul 30, 2019

@Progi1984 ok just waiting for the review :) before the test

Copy link
Contributor

left a comment

Thanks @Progi1984

@eternoendless eternoendless added this to the 1.7.7.0 milestone Aug 1, 2019
@khouloudbelguith

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Hi @Progi1984,
when the wifi is disabled, The PRESTASHOP NEWS block is OK.
image
But when we enable the wifi again, there is no news displayed.
image

I tried when the cache is enabled & cache disabled => Same behavior.

We need to clear cache from the BO => Performance page => to show the exact links
https://drive.google.com/file/d/14ngxoJTQlSPP44JOQmVCvTyxMnbQ1KFc/view

Thanks!

@khouloudbelguith

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Hi @Progi1984,

As discussed with @jolelievre, this a normal behavior

If timeout on the retries, how long do we wait before trying again?
24h

I checked with OPEN_THRESHOLD_SECONDS = 120seconds => OK.

Thanks!

@jolelievre jolelievre merged commit 60035dd into PrestaShop:develop Aug 6, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jolelievre

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Thanks @Progi1984

@Progi1984 Progi1984 deleted the Progi1984:issue12827 branch Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.