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.

Sign up for GitHub

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

Jump to bottom

[#40969] show correct error message in files tab #49

Merged
merged 1 commit into from
Mar 9, 2022
Merged

[#40969] show correct error message in files tab #49

merged 1 commit into from
Mar 9, 2022

Conversation

individual-it
Copy link
Collaborator

@individual-it individual-it commented Feb 18, 2022 •
edited
Loading

This turned into a bit of refactoring for passing around error messages.
Now we have these options that can happen:

When OpenProject server is not reachable
grafik

OpenProject does not support the filter (what is currently always the case, because its not implemented yet)
grafik

No OAuth connection to OpenProject, because the connection was not established yet
grafik

@individual-it individual-it self-assigned this Feb 18, 2022
julien-nc
julien-nc previously requested changes Feb 18, 2022
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

I'm not sure about this. We never show technical errors to the user in Nextcloud but we log them.
So we could display a "human readable" standard error message in the UI but log the real error message as a server-side warning log and optionally in the browser logs.

Why do you want the user to potentially see a CURL error?

lib/Controller/OpenProjectAPIController.php Outdated Show resolved Hide resolved
lib/Service/OpenProjectAPIService.php Outdated Show resolved Hide resolved
@individual-it individual-it force-pushed the task/40969-better-error-messages-in-case-of-openproject-connection-issue branch 2 times, most recently from 3455027 to 2e75531 Compare February 23, 2022 03:22
individual-it
individual-it commented Feb 23, 2022
View reviewed changes
lib/Service/OpenProjectAPIService.php
$body = $response->getBody();
$respCode = $response->getStatusCode();

if ($respCode >= 400) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the response code should never be >= 400 here because that would throw a Client or ServerException and that is handled lower

individual-it
individual-it commented Feb 23, 2022
View reviewed changes
lib/Service/OpenProjectAPIService.php
return [
'error' => $e->getMessage(),
'statusCode' => 404,
];
} catch (Exception $e) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the code should actually never come here, because realistic exceptions are handled higher up, but still I would like to leave that catch just in case

@individual-it
Copy link
Collaborator Author

@eneiluj I've refactored the PR, so that the user only sees "readable" messages, see screenshots in the first post. Please review again

@individual-it individual-it force-pushed the task/40969-better-error-messages-in-case-of-openproject-connection-issue branch from 485a98d to f55dc75 Compare February 24, 2022 06:49
@individual-it individual-it force-pushed the task/40969-better-error-messages-in-case-of-openproject-connection-issue branch from f55dc75 to a666389 Compare March 9, 2022 07:09
@github-actions
Copy link

github-actions bot commented Mar 9, 2022

JS Code Coverage

Coverage after merging task/40969-better-error-messages-in-case-of-openproject-connection-issue into master will be
44.06%
Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   utils.js8.33%0%0%14.29%3–9, 9
src/components
   AdminSettings.vue40.91%100%16.67%50%74–76, 80, 87–88, 90, 93
   OAuthConnectButton.vue92.59%100%83.33%95.24%34
   PersonalSettings.vue33.33%26.32%27.27%38.89%102–103, 103, 103, 106–107, 107, 107, 110–111, 111, 111, 114, 117–118, 120–121, 121–123, 123, 123, 123–124, 129–130, 130, 130, 135, 141, 89–92, 98–99
src/components/settings
   SettingsTitle.vue0%100%0%0%14
src/components/tab
   EmptyContent.vue100%100%100%100%
   SearchInput.vue63.75%53.85%75%63.64%117, 117, 117–118, 120, 125, 125, 125, 144, 152–156, 161–163, 177, 179–180, 182, 184–185, 185, 185, 90
src/views
   Dashboard.vue0%0%0%0%101, 106, 106, 106–107, 109, 115, 119–120, 128, 131, 135–137, 139, 142–143, 146–147, 147–148, 152–153, 153–154, 156, 158–159, 159, 159, 159, 159–161, 161, 161, 161, 161–163, 166, 171, 171, 171, 173–174, 174, 174–175, 177, 177–179, 183, 187, 190, 190, 190, 193, 193, 193, 198, 201, 201, 201, 208, 211, 211, 211, 216, 216, 216, 22, 221, 221, 221, 226, 229, 23, 232, 235, 235, 235, 238, 238, 238, 24, 241, 244, 247, 25–30, 47, 54, 54, 61, 64–65, 77–78, 78, 78, 81, 84, 84, 84–86, 86, 86–88, 88–89, 91, 94, 94, 94–96, 96, 96–98, 98–99
   ProjectsTab.vue80.49%80%83.33%80%74–75, 83, 83, 83–84, 86

@github-actions
Copy link

github-actions bot commented Mar 9, 2022

PHP Code Coverage

Coverage after merging task/40969-better-error-messages-in-case-of-openproject-connection-issue into master will be
44.11%
Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
lib/AppInfo
   Application.php0%100%0%0%49, 51–52, 54–55, 59–60, 63, 70, 74–77, 79–84, 95, 99
lib/BackgroundJob
   CheckNotifications.php0%100%0%0%48, 50, 52–53, 61–62
lib/Controller
   ConfigController.php63.64%100%60%63.86%101–102, 104, 115–116, 118, 216–221, 223–226, 228, 230, 81–82, 84, 86–88, 90–95
   OpenProjectAPIController.php95.24%100%85.71%96.43%157, 78
lib/Dashboard
   OpenProjectWidget.php0%100%0%0%106–109, 61–64, 71, 78, 85, 92, 99
lib/Listener
   LoadSidebarScript.php0%100%0%0%59–61, 65–66, 68, 70, 72–74, 76–78
lib/Notification
   Notifier.php0%100%0%0%100–104, 109–110, 112, 114, 47–50, 60, 69, 80, 82, 85, 87–92, 95, 97
lib/Search
   OpenProjectSearchProvider.php0%100%0%0%102, 109–110, 113–116, 118–119, 123–125, 127–131, 133–135, 138–139, 141–142, 146–147, 149–151, 157–158, 160, 169, 177–183, 192–197, 206, 71–75, 82, 89, 97, 99
   OpenProjectSearchResultEntry.php100%100%100%100%
lib/Service
   OpenProjectAPIService.php62.18%100%66.67%61.80%105–114, 116–119, 122–127, 129–130, 133–134, 150–154, 164–165, 167–171, 173, 182–186, 381–382, 384, 390, 396–401, 403, 433, 471–472, 479, 482–485, 487, 493, 497–499, 95–96
lib/Settings
   Admin.php0%100%0%0%25–26, 33–35, 37, 42–43, 47, 51
   AdminSection.php0%100%0%0%19–20, 29, 39, 48, 55
   Personal.php0%100%0%0%38–41, 48–52, 55, 57, 59–61, 65–66, 70, 74
   PersonalSection.php0%100%0%0%19–20, 29, 39, 48, 55

@individual-it individual-it merged commit bfecce3 into master Mar 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the task/40969-better-error-messages-in-case-of-openproject-connection-issue branch March 9, 2022 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kiranparajuli589 kiranparajuli589 kiranparajuli589 approved these changes

@SwikritiT SwikritiT Awaiting requested review from SwikritiT

@julien-nc julien-nc Awaiting requested review from julien-nc

Assignees

@individual-it individual-it

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@individual-it @julien-nc @kiranparajuli589