Skip to content

Baikal CalDAV error hint#6021

Merged
leog merged 2 commits intomainfrom
chore/baikal-caldav-error-hint
Dec 15, 2022
Merged

Baikal CalDAV error hint#6021
leog merged 2 commits intomainfrom
chore/baikal-caldav-error-hint

Conversation

@leog
Copy link
Copy Markdown
Contributor

@leog leog commented Dec 14, 2022

What does this PR do?

Introduces a better error message when adding CalDAV Baikal integration, which according to #588 it's due to the wrong WebDAV authentication type.

image

Fixes #588

Environment: Staging(main branch) / Production

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How should this be tested?

  • Test A
  • Test B

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't performed a self-review of my own code and corrected any misspellings
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my PR needs changes to the documentation
  • I haven't checked if my changes generate no new warnings
  • I haven't added tests that prove my fix is effective or that my feature works
  • I haven't checked if new and existing unit tests pass locally with my changes

@vercel
Copy link
Copy Markdown

vercel bot commented Dec 14, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Dec 15, 2022 at 1:44PM (UTC)

@leog leog requested a review from a team December 14, 2022 15:47
"impersonate_user_tip": "All uses of this feature is audited.",
"impersonating_user_warning": "Impersonating username \"{{user}}\".",
"impersonating_stop_instructions": "<0>Click Here to stop</0>.",
"impersonating_stop_instructions": "Click here to stop",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Was not being used, so I corrected format and used it

logger.error("Could not add this caldav account", e);
if (e instanceof Error) {
let message = e.message;
if (e.message.indexOf("Invalid credentials") > -1 && url.indexOf("dav.php") > -1) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we know it is baikal and the error is "Invalid credentials", then we introduce the hint

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: .includes is easier to eyes :D

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Might be a good idea to make an abstraction like isBaikal. That would not require any comment and in future too it would be clear what's going on

target="_blank"
className="ml-5 w-32 !p-5">
Go to Admin
</Button>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Providing a button to easily go to the Baikal Admin page to change the wrong value

Copy link
Copy Markdown
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

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

Looks good, only nits are there.

logger.error("Could not add this caldav account", e);
if (e instanceof Error) {
let message = e.message;
if (e.message.indexOf("Invalid credentials") > -1 && url.indexOf("dav.php") > -1) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Might be a good idea to make an abstraction like isBaikal. That would not require any comment and in future too it would be clear what's going on

"//" +
parsedUrl.hostname +
(parsedUrl.port ? ":" + parsedUrl.port : "") +
"/admin/?/settings/standard/";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Can be simplified as ${parsedUrl.origin}/admin/?/settings/standard

severity="error"
title={errorMessage}
actions={
errorActionUrl !== "" ? (
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: errorActionUrl ?

Copy link
Copy Markdown
Member

@alishaz-polymath alishaz-polymath left a comment

Choose a reason for hiding this comment

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

🚀

@alishaz-polymath
Copy link
Copy Markdown
Member

Agreed with @hariombalhara NITs. Specially for the code readability suggestions (adding the isBaikal check)

@leog
Copy link
Copy Markdown
Contributor Author

leog commented Dec 15, 2022

@hariombalhara @alishaz-polymath thanks for your feedback, will tackle on a follow up PR soon enough

@leog leog enabled auto-merge (squash) December 15, 2022 13:34
@leog leog merged commit a638708 into main Dec 15, 2022
@leog leog deleted the chore/baikal-caldav-error-hint branch December 15, 2022 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide more informed Error message when CalDAV connection fails [BAIKAL]

3 participants