feat: adding AdSlot and Newsletter service#5
Conversation
… code that helps for equality checking for snapshots when running tests
…pshots when running tests
tzanetti-liveintent
left a comment
tzanetti-liveintent
left a comment
There was a problem hiding this comment.
Mostly looks good. Left a few questions to be addressed.
I think one use case it would be good to get on test is to update sizes as well.
| run: composer i | ||
| - name: Run linter | ||
| run: ./vendor/bin/php-cs-fixer fix --dry-run --diff --format=checkstyle | cs2pr | ||
| run: ./vendor/bin/php-cs-fixer fix --dry-run --diff --format=checkstyle --allow-risky=yes | cs2pr |
There was a problem hiding this comment.
Question: this flag sounds risky =)
Why do we need it? And is this standard for company or just this one time off?
There was a problem hiding this comment.
That was added after running into this error https://github.com/LiveIntent/sdk-php/runs/5677792710?check_suite_focus=true
There was a problem hiding this comment.
I'm not sure if we can handle it differently or not, but that's why I added it.
|
|
||
| For detailed documentation see [here](https://laravel.com/docs/8.x/http-client#testing). | ||
|
|
||
| ## Development |
There was a problem hiding this comment.
Great job splitting up docs!
| ? json_decode(collect($request->data())->flip()->first(), true) | ||
| : $request->data(); | ||
| // Initialize data when not json | ||
| $data = $request->data(); |
There was a problem hiding this comment.
Q: Do you think we'll ever need to do that again? If so, then maybe move this to a common/shared function?
There was a problem hiding this comment.
I haven't seen another place that we do this. I'm not sure if we'll ever need to do this again.
| /** @return string */ | ||
| public static function adSlotHash() | ||
| { | ||
| return '60220c073cc811ec941d0ab243175da9'; |
There was a problem hiding this comment.
Q: Is any of this data depended on ids being stored in database or are these mocked?
| 'client_id' => env('CLIENT_ID'), | ||
| 'client_secret' => env('CLIENT_SECRET'), | ||
| 'base_url' => env('LI_BASE_URL', 'http://localhost:33001'), | ||
| 'base_url' => env('LI_BASE_URL', 'http://localhost:3000'), |
2 participants
No description provided.