- Issue created by @balintbrews
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ Improve client side error handling Fixed is in โ the next step is โจ [PP-1] Log client-side errors Postponed , which this is a blocker for.
- First commit to issue fork.
- Merge request !179#3467843: Back-end route to allow logging from the UI โ (Merged) created by omkar-pd
- Status changed to Needs review
6 months ago 3:45pm 19 August 2024 - Status changed to Needs work
6 months ago 4:09pm 19 August 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Thanks, @omkar-pd, looking good! This still needs a functional test ๐
- ๐ฎ๐ณIndia omkar-pd
Created my first tests.๐ Probably will need some changes.
- Assigned to pooja_sharma
- Status changed to Needs review
6 months ago 12:25pm 23 August 2024 Addressed test coverage issue , test passed on local however left there one comment on MR for test coverage need suggestion here , fixed undefined variable errors for newly added route, rebased the MR & resolved conflicts.
Please review, moving NR
- Issue was unassigned.
- Status changed to Needs work
6 months ago 8:25am 26 August 2024 Thanks @wim-leers for recognizing efforts happy to contribute , got your concerns regarding test coverage feedback after removing standard profile code, test resource usage goes from Time:
00:16.673
, Memory: 4.00 MB to Time:00:05.687
, Memory: 4.00 MB which eliminates the half of test execution time (much faster).I m here for contributing for test coverage as per request @omkar-pd, initially much of the work done by him so letting him to cover up pending feedback.
- Status changed to Needs review
6 months ago 11:42am 26 August 2024 - ๐ฎ๐ณIndia omkar-pd
Thank you @wim-leers and @pooja_sharma.
I've addressed the feedback given. Moving this to NR.
- Assigned to wim leers
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
This should wait for ๐ Also validate request bodies against the OpenAPI spec Downport to land โ it makes
openapi.yml
more maintainable.(I expect to land in the next hour or so though ๐)
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ Also validate request bodies against the OpenAPI spec Downport is in, rebasingโฆ
- Assigned to traviscarden
- Status changed to RTBC
6 months ago 9:51am 30 August 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
LGTM!
But since I requested @traviscarden to become the
CODEOWNER
for/openapi.yml
at #3466555-21: Also validate request bodies against the OpenAPI spec โ .3, I'd like him to do the final review ๐ - Issue was unassigned.
- Status changed to Needs work
6 months ago 9:18pm 30 August 2024 - ๐บ๐ธUnited States traviscarden
Nice contribution! I've requested some changes on the MR.
- ๐ฎ๐ณIndia omkar-pd
@wim-leers, @traviscarden
Need suggestions for this.
https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Will review/merge in upstream to get this back on track, because ๐ OpenAPI validation errors must be provided as a JSON response RTBC has landed and this blocks the subsequent steps ๐ค
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
To answer #23:
There was 1 error:
1) Drupal\Tests\experience_builder\Functional\LogControllerTest::testLogController
Exception: Warning: Undefined array key "invalid_log_level"
Drupal\Core\Logger\LoggerChannel->log()() (Line: 123)
/builds/issue/experience_builder-3467843/web/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:55shows that there's a bug in the controller logic:
if (empty($error_level)) { return new JsonResponse(['error' => 'Log level is required'], 400); } // Log the error. $this->logger->log($error_level, $error_message);
๐ That doesn't verify the log level is valid, only that one is specified.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
@larowlan's https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... still needs to be addressed.
Almost there! ๐
- First commit to issue fork.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Still needs the tests that @larowlan suggested.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Final review pass, fixed my own nits. Looked great!
I actually don't quite like that this is now a unit test instead of a functional test. Because it doesn't prove the route actually works, only that the controller works.
But โฆ โจ [PP-1] Log client-side errors Postponed will have to add an end-to-end test, and that's even better.
So: RTBC! This now only needs MR approval from @traviscarden, so assigning to him.
- ๐บ๐ธUnited States traviscarden
Nice! I prettified
openapi.yml
and approved it. -
wim leers โ
committed 9480067e on 0.x authored by
omkar-pd โ
Issue #3467843 by omkar-pd, pooja_sharma, wim leers, traviscarden,...
-
wim leers โ
committed 9480067e on 0.x authored by
omkar-pd โ
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Yay! Merged and unpostponing โจ [PP-1] Log client-side errors Postponed ๐
Automatically closed - issue fixed for 2 weeks with no activity.