Skip to content

Conversation

@pierre-lehnen-rc
Copy link
Contributor

Proposed changes

This PR aims to simplify the data importers process by storing the data of any importer in a standard format that can be converted by the same process.

Issue(s)

How to test or reproduce

Screenshots

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Hotfix (a major bugfix that has to be merged asap)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Changelog

Further comments

@lgtm-com
Copy link

lgtm-com bot commented Jul 23, 2020

This pull request introduces 1 alert when merging 18c446e into def02e9 - view on LGTM.com

new alerts:

  • 1 for Template syntax in string literal

@wreiske
Copy link
Contributor

wreiske commented Jul 24, 2020

This is very cool. I'd like to help drive connecting this up with another project aimed at simplifying access to third-party data providers such as Google Contacts, GSuite, ADP, BambooHR, BreezyHR, Salesforce, Workday, etc. You can read about it here https://github.com/easybread/easybread/tree/develop/packages/core#easybread and see the documentation https://github.com/easybread/easybread/wiki/GeneralConcepts

This would make it really cool for rocket.chat users that have an HR or directory like BambooHR, ADP, Gsuite, but no AD or LDAP.
With this method, administrators wouldn't have to manually create users in RC and could sync right up with their HR provider without needing LDAP or AD.

Also, easybread plans to support LDAP and AD as an adapter soon so we could potentially also take over RC's LDAP user sync code with the easybread adapter. Maybe even slack and other adapters could be implemented in easybread and then it would all just be in one module in RC and as easybread implements more adapters they instantly become available with a npm update of easybread on RC.

I have a freelance developer that has done the majority of the work on easybread. We would be happy to work with the Rocket.Chat team to build out the UI functionality and having easybread importers available as part of the setup wizard and also in the administration dashboard. Imagine setting up a new Rocket.Chat server, clicking on an import from existing directory button, and being able to select any third party provider without any need for Rocket.Chat's team to maintain the adapters.

image

@lgtm-com
Copy link

lgtm-com bot commented Jul 24, 2020

This pull request introduces 5 alerts when merging 71c7aea into 4e176de - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class
  • 1 for Template syntax in string literal

@lgtm-com
Copy link

lgtm-com bot commented Jul 28, 2020

This pull request introduces 5 alerts when merging 7291996 into ccc323f - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class
  • 1 for Template syntax in string literal

@geekgonecrazy
Copy link
Contributor

i'll create a discussion on open. I don't want to let this sit un-answered too long

Removed changes from PR #20216 that were made on the old structure - they need to be re-implemented on the new slack importer.

# Conflicts:
#	app/importer-slack/server/importer.js
@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2021

This pull request introduces 5 alerts when merging 91be6ad into f50d745 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class
  • 1 for Template syntax in string literal

@geekgonecrazy
Copy link
Contributor

Any particular reason we haven’t moved forward and merged?

@pierre-lehnen-rc
Copy link
Contributor Author

Any particular reason we haven’t moved forward and merged?

It's missing a few things, but I'll finish it for this month's release.

@lgtm-com
Copy link

lgtm-com bot commented Apr 20, 2021

This pull request introduces 1 alert when merging 687423c into dd340e0 - view on LGTM.com

new alerts:

  • 1 for Template syntax in string literal

@pierre-lehnen-rc pierre-lehnen-rc marked this pull request as ready for review April 20, 2021 20:59
this.reloadCount();
const started = Date.now();
const startedByUserId = Meteor.userId();
console.log(importSelection);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
console.log(importSelection);

@lgtm-com
Copy link

lgtm-com bot commented Apr 20, 2021

This pull request introduces 1 alert when merging 4bfdaf4 into 880546b - view on LGTM.com

new alerts:

  • 1 for Template syntax in string literal

@sampaiodiego sampaiodiego added this to the 3.14.0 milestone Apr 20, 2021
@@ -0,0 +1,17 @@
export interface IImportUser {
// #ToDo: Remove this _id, as it isn't part of the imported data
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops

this.reloadCount();
const started = Date.now();
const startedByUserId = Meteor.userId();
console.log(importSelection);
Copy link
Member

Choose a reason for hiding this comment

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

😬

fn();
} catch (e) {
console.log('errror', e); // TODO: Remove
console.log('error', e); // TODO: Remove
Copy link
Member

Choose a reason for hiding this comment

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

I think this either should be a logger call or a console.error


if (roomData.t === 'd') {
if (members.length < roomData.users.length) {
console.warn('One or more imported users not found: ${ roomData.users }');
Copy link
Member

Choose a reason for hiding this comment

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

thx to LTGM.. this log will not show what is expected =)

btw, any reason to not use Logger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not anymore, just didn't update this part of the code.

@sampaiodiego sampaiodiego merged commit 14f7c3d into develop Apr 21, 2021
@sampaiodiego sampaiodiego deleted the ts-importers branch April 21, 2021 04:14
@sampaiodiego sampaiodiego mentioned this pull request Apr 28, 2021
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.

6 participants