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

Add Mailgun region setting #1850

Closed
wants to merge 9 commits into from

Conversation

@vingrad
Copy link

vingrad commented Aug 19, 2019

**Fixes #1834 **

Changes proposed in this pull request:

Support for the new option "endpoint setting" for mailgun.

Reviewers should focus on:

Screenshot

image

Confirmed

  • Frontend changes: tested on a local Flarum installation.

Required changes:

Vladimir Vinogradov added 3 commits Aug 19, 2019
Vladimir Vinogradov
Fixes #1834
Add Mailgun region setting
Vladimir Vinogradov
Fixes #1834
remove unneeded }
Copy link
Member

luceos left a comment

I approve of the changes, but noticed the comments in the MailGun driver are incorrect. It mentions domain to be for the API endpoint, but that's not the case. Either we drop those comments or we fix them, I'd prefer dropping.

Copy link
Member

franzliedke left a comment

Thank you very much for tackling this issue!

I have two things I would like to see changed, then we can get this merged.

js/src/admin/components/MailPage.js Outdated Show resolved Hide resolved
js/src/admin/components/MailPage.js Outdated Show resolved Hide resolved
@luceos luceos dismissed their stale review Aug 21, 2019

franz remarks are valid

Vladimir Vinogradov and others added 5 commits Aug 25, 2019
Vladimir Vinogradov
#1834
Add Mailgun region setting
Vladimir Vinogradov
Vladimir Vinogradov
fixes #1834
better formatting
@vingrad

This comment has been minimized.

Copy link
Author

vingrad commented Aug 25, 2019

I did some changes. Now it's more generic. Please check again. @franzliedke @luceos
But I did not found a good way to passthrough Translation class to the MailgunDriver. So you will see now only hostnames for endpoints, but I think, that it's also Ok.

Copy link
Member

franzliedke left a comment

Thanks, we're making progress! A couple of questions...

@@ -18,7 +18,9 @@ export default class MailPage extends Page {
this.values = {};

const settings = app.data.settings;
this.fields.forEach(key => this.values[key] = m.prop(settings[key]));
console.log('settings', settings);

This comment has been minimized.

Copy link
@franzliedke

franzliedke Aug 28, 2019

Member

Please remove this line.

This comment has been minimized.

Copy link
@vingrad

vingrad Aug 28, 2019

Author

Sorry, done.

}
);
}

This comment has been minimized.

Copy link
@franzliedke

franzliedke Aug 28, 2019

Member

Does this change any functionality or did you just change the loop?

This comment has been minimized.

Copy link
@vingrad

vingrad Aug 28, 2019

Author

It's just other loop, because we need to iterate over the object.

{
return [];
return (object) [];

This comment has been minimized.

Copy link
@franzliedke

franzliedke Aug 28, 2019

Member

What's the benefit of using an object over an array here?

This comment has been minimized.

Copy link
@vingrad

vingrad Aug 28, 2019

Author

We need an object for additional parameters like the type of the form element and also for possible values.

image

This comment has been minimized.

Copy link
@franzliedke

franzliedke Oct 1, 2019

Member

@vingrad Can you expand on that and explain the why? That structure is perfectly valid as an array as well, and it should serialize to JSON in the same way?

@datitisev datitisev mentioned this pull request Nov 11, 2019
2 of 3 tasks complete
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 10, 2020

I've taken care of the remaining comments, simplified everything and squash-merged it into master in 4c89e2e.

Thanks for your efforts, @vingrad !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.