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 the message sent to the popup when connecting using oauth #7052

Open
wants to merge 1 commit into
base: hotfix-7.10.x
Choose a base branch
from

Conversation

isleshocky77
Copy link
Contributor

Description

Currently, the string_format() escapes the string in a way which makes a
javascript error.

Motivation and Context

When attempting to use a oauth connector I was getting a javascript error when trying to connect using EAPM.

How To Test This

  • Create a connector which uses OAuth and has eapm enabled
  • Enable the connector and set it up
  • Go and create an External Account using that connector and click connect
  • Note javascript error

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

@Dillon-Brown Dillon-Brown added the PR:Community Contribution These are contribution made by the community label Mar 14, 2019
@codecov-io
Copy link

codecov-io commented Mar 14, 2019

Codecov Report

Merging #7052 into hotfix will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             hotfix    #7052      +/-   ##
============================================
- Coverage      10.9%    10.9%   -0.01%     
  Complexity    42309    42309              
============================================
  Files          3385     3385              
  Lines        248654   248654              
============================================
- Hits          27115    27114       -1     
- Misses       221539   221540       +1

gymad
gymad previously approved these changes Mar 25, 2019
@@ -125,7 +125,7 @@ protected function post_save()
// It's OAuth, we have to handle this specially.
// We need to create a new window to handle the OAuth, and redirect this window back to the edit view
// So we will handle that in javascript.
$popup_warning_msg = string_format($GLOBALS['mod_strings']['LBL_ERR_POPUPS_DISABLED'], array($_SERVER['HTTP_HOST']));
$popup_warning_msg = strtr($GLOBALS['mod_strings']['LBL_ERR_POPUPS_DISABLED'], array( '{0}' => $_SERVER['HTTP_HOST']));
Copy link
Contributor

Choose a reason for hiding this comment

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

Very "elegant" solution. Also there is the LangText::get() function which does something very similar for associative language text replacement at https://github.com/salesagility/SuiteCRM/blob/master/include/LangText.php#L329
I am not quite sure if it solves this issue with indexed parameters like "{0}". However, this PR solves the issue so I'll approve it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm having a hard time seeing it from quickly looking at that function.. I'm not sure what it does. It would be great if it was better documented in the docBlock. I'll look and see how it's used elsewhere and if there is any testing showing how's it's used; however, are you signing off on my change as is or recommending I change it?

@isleshocky77
Copy link
Contributor Author

@Dillon-Brown Just noticed you added a label to this.. what does it mean?

@isleshocky77
Copy link
Contributor Author

@Dillon-Brown Rebased on latest hotfix

@Dillon-Brown
Copy link
Contributor

@isleshocky77 Should this not be to hotfix-7.10.x or is it hotfix specific?

@isleshocky77
Copy link
Contributor Author

@Dillon-Brown I'm running 7.11

Currently, the string_format() escapes the string in a way which makes a
javascript error.
@isleshocky77 isleshocky77 changed the base branch from hotfix to hotfix-7.10.x September 23, 2019 14:50
@isleshocky77
Copy link
Contributor Author

@Dillon-Brown Rebased on hotfix-7.10.x

@SuiteBot
Copy link

SuiteBot commented Aug 27, 2020

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR:Community Contribution These are contribution made by the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants