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.

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

[NEW] Drupal oAuth Integration for Rocketchat #6632

Merged
merged 13 commits into from Apr 10, 2017

Conversation

Lawri-van-Buel
Copy link
Contributor

@RocketChat/core

Updates issue #1921

This adds an easy to use Drupal oauth integration path and is the rocketchat part of the new rocketchat drupal integration module on drupal.org (version 7.x-1.1) that will soon be released. It will allow you to use Drupal as a Login provider for your rocketchat users.

@CLAassistant
Copy link

CLAassistant commented Apr 7, 2017

CLA assistant check
All committers have signed the CLA.

@Lawri-van-Buel Lawri-van-Buel mentioned this pull request Apr 7, 2017
@geekgonecrazy
Copy link
Member

@Lawri-van-Buel thanks for contributing! Do you happen to have a link to the Rocket.Chat Drupal integration module you mentioned? I'd love to check it out.

@@ -137,6 +137,7 @@ rocketchat:crowd@1.0.0
rocketchat:custom-oauth@1.0.0
rocketchat:custom-sounds@1.0.0
rocketchat:dolphin@0.0.2
rocketchat:drupal@0.0.1
Copy link
Member

Choose a reason for hiding this comment

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

Can you undo all other changes to this file but this line here? Ideally this PR shouldn't be updating other packages, it should only be adding the code needed for drupal. We'd handle the updating of packages seperately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree I though I had done just that, Will do with the new update as its base.

.meteor/packages Outdated
jquery@1.11.10
less@2.7.9
logging@1.1.17
meteor-base@1.0.4
mobile-experience@1.0.4
mongo@1.1.15
mongo@1.1.16
Copy link
Member

Choose a reason for hiding this comment

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

Can you undo changes to this file? Like mentioned in the other comment, ideally we'd handle these types of updates in a seperate PR. This should only about adding drupal. Thx 👍

@Lawri-van-Buel
Copy link
Contributor Author

Lawri-van-Buel commented Apr 7, 2017

@geekgonecrazy My drupal 7 module is on the Drupal.org Repo, I am writing some documentation before I will release the drupal module (should be sometime the coming week) as for the development version its the latest drupal dev version on drupal.org

@Lawri-van-Buel
Copy link
Contributor Author

@geekgonecrazy can you confirm the issues are resolved? or have I missed something?

.meteor/packages Outdated
@@ -170,3 +170,5 @@ yasaricli:slugify
yasinuslu:blaze-meta
deepwell:bootstrap-datepicker2
rocketchat:google-natural-language
rocketchat:drupal
rocketchat:i18n
Copy link
Member

Choose a reason for hiding this comment

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

Was it necessary to add this? I think our other packages might just add this as a dependency.

@RocketChat/core is there a reason it's not in the packages list?

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 sure. I just used the build example and added rocketchat:drupal using the meteor add rocketchat:drupal command. that I take it added the lines to the package. I am new to meteor, but expierenced in drupal so do not be afraid to tell me i did something wrong ;) I prefer to do it right than muddle on in a 'wrong' way.

@geekgonecrazy
Copy link
Member

@Lawri-van-Buel awesome I'll check it out. Just will want to get feedback from more of the team, then get it merged 👍

@Lawri-van-Buel
Copy link
Contributor Author

great I'll await yours and their review

@Lawri-van-Buel
Copy link
Contributor Author

Is this now ready to be merged or do you want me to change something still?

@geekgonecrazy
Copy link
Member

geekgonecrazy commented Apr 10, 2017

@sampaiodiego / @rodrigok only change i'm not sure about is that rocketchat:i18n was added to .meteor/packages it wasn't before.

@sampaiodiego
Copy link
Member

sampaiodiego commented Apr 10, 2017

@geekgonecrazy only top level packages are added to .meteor/packages file.. since rocketchat:i18n is only added by rocketchat:lib it is not there.

but this doesn't explain why it was added after a meteor add rocketchat:drupal..

btw, I don't think by just having the file packages/rocketchat-drupal/i18n/en.i18n.json it would be used for translations.. I would recommend move this translation strings over packages/rocketchat-i18n/i18n/en.i18n.json file since we don't have packages specific translations at the moment 😞

@Lawri-van-Buel
Copy link
Contributor Author

Updated my fork with the removal of the i18n line in .meteor/packages and moving the translatable strings to the i18n module.

@engelgabriel engelgabriel added this to the 0.55.0 milestone Apr 10, 2017
@engelgabriel
Copy link
Member

@sampaiodiego please let's include this on the next candidate release.

@engelgabriel engelgabriel merged commit 74db878 into RocketChat:develop Apr 10, 2017
@designgurudotorg
Copy link
Contributor

This is amazing! Great work :)

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.

None yet

6 participants