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

Fix: wrong permission route #570

Merged
merged 3 commits into from
Apr 10, 2020
Merged

Fix: wrong permission route #570

merged 3 commits into from
Apr 10, 2020

Conversation

dinhtungdu
Copy link
Contributor

Description of the Change

The permission route in #245 which was merged to develop is wrong, that causes creating external connections failed. This PR fixes the URL and adds more checks to prevent potential warnings and errors.

Alternate Designs

Benefits

Possible Drawbacks

Verification Process

See the tests pass.

Checklist:

  • I have read the CONTRIBUTING document.
  • 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 change.
  • All new and existing tests passed.

Applicable Issues

Changelog Entry

@dinhtungdu dinhtungdu self-assigned this Mar 27, 2020
@dinhtungdu dinhtungdu added the type:bug Something isn't working. label Mar 27, 2020
@jeffpaul
Copy link
Member

I wonder if this may be why @dkotter and @helen are having different results in testing #368, assuming Helen pulled develop down after #245 was merged and Darin before?

@dinhtungdu
Copy link
Contributor Author

@jeffpaul I don't think so, that issue relates to Application Password load order.

Darin Kotter added 2 commits April 2, 2020 10:09
… we have a proper javascript array to process. Minor code cleanup
@dkotter
Copy link
Collaborator

dkotter commented Apr 2, 2020

@dinhtungdu Testing this out, I had an issue where it would connect to an external connection but wouldn't give me the proper message (gave me the yellow light instead of the green). Tracked down the issue and the problem was with the array we return to our ajax function. The array keys needed reset, otherwise it ended up as a javascript object and not an array, which was breaking our javascript functionality.

I've made that fix, as well as some minor code formatting fixes (that were mostly from the original PR). This looks good to me now.

@dkotter
Copy link
Collaborator

dkotter commented Apr 2, 2020

And just for performance numbers, on my local install, I saw these external connection test requests go from 8-9 seconds down to 1.5-2 seconds, so a pretty nice improvement there.

@dkotter dkotter merged commit d57427a into develop Apr 10, 2020
@dkotter dkotter deleted the fix/wrong-permission-url branch April 10, 2020 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants