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

Updating intercom functionality #1734

Merged
merged 17 commits into from
Jun 27, 2017
Merged

Updating intercom functionality #1734

merged 17 commits into from
Jun 27, 2017

Conversation

willdoran
Copy link
Contributor

@willdoran willdoran commented May 9, 2017

This change is Reviewable

$di->set('site.intercomAppToken', function() use ($di) {
return Kohana::$config->load('site.intercomAppToken');
$di->set('thirdparty.intercomAppToken', function() use ($di) {
return Kohana::$config->load('thirdparty.intercomAppToken');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest just calling getenv() here rather than using Kohana config

public function handle(EventInterface $event, $user = null)
{

$config = service('repository.config')->get('thirdparty');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to use the repo here, just service('thirdparty.intercomapptoken')
The repo is only really necessary for things that are use editable

{

$config = service('repository.config')->get('thirdparty');
$domain = Kohana::$config->load('site.client_url');
Copy link
Contributor

Choose a reason for hiding this comment

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

You might be able to use service('site') here. I think it'll give you domain.ushahidi.io or similar. But relies on HOST being set or an actual HTTP request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What will that do in a non .io setup, it seems to be unset if not multisite


try {
// Create company with current date if it does not already exist
if (!$config->intercomCompanyId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When would this happen? ie. why wouldn't we have created the company already?

@@ -64,7 +64,11 @@ public function create(Entity $entity)
'created' => time(),
'password' => $this->hasher->hash($entity->password),
];
return parent::create($entity->setState($state));
$entity->setState($state);
if ($entity->role === 'admin') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally this check happens in the listener too. Then the event is reusable for future integrations that might want all users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any change on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that's changed

$client = new IntercomClient($intercomAppToken, null);

try {
$client->users->update([
Copy link
Contributor

Choose a reason for hiding this comment

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

indenting is weird here?

{
if($user && $user->role === 'admin') {
$intercomAppToken = getenv('INTERCOM_APP_TOKEN');
$domain = service('site');
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return quakemap.api.ushahidi.io or just an empty string if we're not on an ushahidi.io deployment ... was that what you expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is intended to track .io I think that's ok, I'll test it with a blank string to make that it behaves as expected.

{
$intercomAppToken = service('site.intercomAppToken');
$this->config_repo = $config_repo;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is never used?

@@ -0,0 +1,30 @@
<?php defined('SYSPATH') OR die('No direct access allowed.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we need this file at all anymore?

@@ -64,7 +64,11 @@ public function create(Entity $entity)
'created' => time(),
'password' => $this->hasher->hash($entity->password),
];
return parent::create($entity->setState($state));
$entity->setState($state);
if ($entity->role === 'admin') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any change on this?

rjmackay
rjmackay previously approved these changes Jun 5, 2017
Copy link
Contributor

@rjmackay rjmackay left a comment

Choose a reason for hiding this comment

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

Looks alright. Flagged some minor clean up in the comments.

Anything further could probably happen in the lumen branches.

rjmackay
rjmackay previously approved these changes Jun 7, 2017
Copy link
Contributor

@rjmackay rjmackay left a comment

Choose a reason for hiding this comment

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

Ship it!

@willdoran willdoran merged commit cbb5698 into develop Jun 27, 2017
@willdoran willdoran deleted the 1733-improve-api-intercom branch May 4, 2018 19:50
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.

2 participants