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

Reimplementing bots #119

Merged
merged 23 commits into from
May 20, 2021
Merged

Reimplementing bots #119

merged 23 commits into from
May 20, 2021

Conversation

tjarbo
Copy link
Owner

@tjarbo tjarbo commented May 9, 2021 •
edited
Loading

As a Developer
I want to have a modular implementation of connectors
so I can easily add new connectors

Description

This PR is linked to #116. I have implemented a new connectorService which handles the publishing of new notifications or update connectors. If no connector is stored in the database, the .env-vars will be used to create a new connector. This can be used to setup and use the service without visiting the dashboard. Now it is possible to add further connector plugins like email, slack, matrix or webhooks.

In was not possible to refactor existing endpoints to the new implementation. Therefore I will do/fix this in separated user stories, because that would be to much for a single user story. The following problems are address in the user stories:

Another topic are unit tests: Unfortunately it was not possible to find a great solution to test an abstract class without rewriting tests again and again (for further plugins). So I skipped all tests that are effected by that.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Checklist

  • [ ] Make sure you are requesting to main. Don't request to release branch!
  • I have linked the related issue.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@tjarbo tjarbo added 👓 need review This issue or pull request needs to be reviewed ⚡ improvement Verbesserung bestehender Funktionen labels May 9, 2021
@tjarbo tjarbo added this to the Version 2.0 milestone May 9, 2021
@tjarbo tjarbo requested review from p-fruck and antonplagemann May 9, 2021 20:26
@tjarbo tjarbo self-assigned this May 9, 2021
p-fruck
p-fruck requested changes May 10, 2021
View reviewed changes
Copy link
Collaborator

@p-fruck p-fruck left a comment

Choose a reason for hiding this comment

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

Couln't find any missing newlines 😞

packages/backend/src/controllers/connectors/logger.ts Outdated Show resolved Hide resolved
packages/backend/src/controllers/connectors/logger.ts Outdated Show resolved Hide resolved
packages/backend/src/controllers/connectors/logger.ts Outdated Show resolved Hide resolved
packages/backend/src/controllers/connectors/plugins/discordBot.class.ts Outdated Show resolved Hide resolved
packages/backend/src/controllers/connectors/plugins/discordBot.class.ts Outdated Show resolved Hide resolved
packages/backend/src/controllers/connectors/service.ts Outdated Show resolved Hide resolved
packages/backend/src/controllers/connectors/service.ts Outdated Show resolved Hide resolved
packages/backend/src/controllers/connectors/service.ts Outdated Show resolved Hide resolved
packages/backend/src/controllers/connectors/service.ts Outdated Show resolved Hide resolved
@antonplagemann
Copy link
Collaborator

Hi there :-)
Some questions that arose while testing:

  • Web GUI is in german, but error notification 'user not found' or 'user not registered' is in english?
  • What if I dont have a security key?
  • Why is an unsuccessful registration attempt (aborted WebAuthn dialog) saved to the database?
    grafik
    grafik
  • Why is the statusboard currently empty?
    Doing a code review later ;-)

@tjarbo
Copy link
Owner Author

tjarbo commented May 11, 2021

Thank you for your review!

@p-fruck
I will put all requested changes into one commit if it's okay

@antonplagemann
Yes, you are right. The problem with mixed languages are caused by our ongoing i18n. I will create missing user stories which address this problem, but they will be part of Version 3

What happen if a user has no security key ? Hopefully this should not be a thing. Because many computers nowadays have a TPM that is used e.g. for Windows Hello and even all modern Smartphones can handle FIDO2.
But of course there will be people, who have no access to FIDO. Than an investment into a security key is certainly well spent.

The status board is currently empty because it can not handle the new response. But this in scope of the next user story (I should start to write them down instead of keeping them in mind 😅)

antonplagemann
antonplagemann requested changes May 13, 2021
View reviewed changes
Copy link
Collaborator

@antonplagemann antonplagemann left a comment

Choose a reason for hiding this comment

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

Looks fantastic 🔥 👍 😊

packages/backend/src/controllers/connectors/schemas/connectorLogItem.schema.ts Outdated Show resolved Hide resolved
This was referenced May 18, 2021
Closed
Closed
@tjarbo tjarbo changed the title WIP: Reimplementing bots Reimplementing bots May 18, 2021
@tjarbo tjarbo requested review from p-fruck and antonplagemann May 18, 2021 19:05
antonplagemann
antonplagemann requested changes May 18, 2021
View reviewed changes
packages/backend/src/controllers/connectors/plugins/connectorPlugin.class.ts Outdated Show resolved Hide resolved
packages/backend/tests/connectors/logger.spec.ts Outdated Show resolved Hide resolved
packages/backend/tests/connectors/logger.spec.ts Outdated Show resolved Hide resolved
packages/backend/tests/connectors/logger.spec.ts Outdated Show resolved Hide resolved
packages/backend/tests/connectors/logger.spec.ts Outdated Show resolved Hide resolved
packages/backend/tests/connectors/logger.spec.ts Outdated Show resolved Hide resolved
packages/backend/tests/connectors/logger.spec.ts Outdated Show resolved Hide resolved
packages/backend/tests/connectors/logger.spec.ts Outdated Show resolved Hide resolved
@tjarbo tjarbo merged commit 9b390a1 into version-2 May 20, 2021
@tjarbo tjarbo deleted the feature/issue-116 branch May 20, 2021 13:33
@tjarbo tjarbo mentioned this pull request Jun 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antonplagemann antonplagemann antonplagemann approved these changes

@p-fruck p-fruck p-fruck approved these changes

Assignees

@tjarbo tjarbo

Labels
👓 need review This issue or pull request needs to be reviewed ⚡ improvement Verbesserung bestehender Funktionen
Projects
None yet
Milestone
Version 2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@tjarbo @antonplagemann @p-fruck