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: 'cancel' button redirects to console instead of portal #387

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

jbostoen
Copy link
Contributor

Before:

  1. Sign in to the portal, navigate to https://demo.combodo.com/simple/pages/exec.php/object/create/UserRequest?exec_module=itop-portal-base&exec_page=index.php&portal_id=itop-portal , press Cancel.
  2. User with console permissions is redirected to the back end instead of staying in the portal.

After PR: user with console permissions should stay in the portal. Either a simple 'go back 1 page' is executed, or the user is redirected to the portal's home page.

@Molkobain Molkobain self-assigned this Jan 15, 2023
@Molkobain Molkobain added the bug Something isn't working label Jan 15, 2023
// Try to close the window
window.close();

if(window.history.length > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

window.history.length greater than 1 means no history? Seems counter intuitive. Isn't it equals 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely right, should have been == 1 (it's always at least 1 = current location)

Comment on lines 438 to 445
var regexPortal = new RegExp('(pages\\/exec\\.php)(.*?)(\\?exec_module=itop-portal-base&exec_page=index\\.php&portal_id=.*?)(&|$)');
var aRegexMatch = window.location.href.match(regexPortal);

if(aRegexMatch !== null) {

sHomepageUrl += aRegexMatch[1] + aRegexMatch[3];

}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure hacking the URL through the regex is the best approach here. To me it's more like the this.options.base_url isn't set properly in the first place. It should be set with app['combodo.portal.base.absolute_url'] in datamodels/2.x/itop-portal-base/portal/templates/bricks/object/mode_create.html.twig

Copy link
Contributor

Choose a reason for hiding this comment

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

True, also some customers might have different path e.g. set up.

For example they use another portal base or portal_id is not the third argument or the portal url is rewritten..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but I think it will be a minority? (Perhaps you guys have a better view on that).

It addresses the most common implementation. About the order of arguments: I could rewrite it like that, but I'm not sure if there's a different order somewhere in the native implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, is the RegEx needed? The filename (portal_form_handler.js) kind of suggests we are always in the portal?

Copy link
Contributor

Choose a reason for hiding this comment

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

I explained the fix as we would accept it in the first message :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely needs the portal ID parameter. The other parts I added to be more certain of where it was used and to re-use in the final URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Molkobain so I should just change this line to use app['combodo.portal.base.absolute_url'] instead in the create object Twig template?

base_url: "{{ app['combodo.absolute_url'] }}",

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I updated the PR to be a mix of @Molkobain's suggestion and also just some basic 'go back' logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that new behavior will need to be tested / validated. We'll see that during next functional review

@Hipska
Copy link
Contributor

Hipska commented Jan 16, 2023

I'm wondering if this now also messes up when the form was created by going to the services browser. As if I do that, clicking cancel on the modal, the modal is closed. If I make the form open as a new page by right clicking on the link, It just closes the window by clicking on cancel (Safari & FF 102) So in this scenario, everything works as expected.

@Molkobain
Copy link
Contributor

Accepted during technical review.

@Molkobain
Copy link
Contributor

Discussed during functional review:

  • Fix of the URL passed to the widget is accepted.
  • Change on the JS to go back to the previous page if any history is available is rejected. I missed it during the technical review, but this is done in the "apply close rule" function which is meant to close the page/form. Any redirection "back" to the previous page should be handle by the "back" redirection rule and is out of scope of this PR.

So long story short, revert the changes on the JS file and the PR will be good to merge :)

@jbostoen
Copy link
Contributor Author

Thanks for the feedback!

Well, it doesn't make sense the page would be "closed" at some point.

Image this scenario:

  1. User opens the portal. ( https://demo.combodo.com/simple/pages/exec.php/?exec_module=itop-portal-base&exec_page=index.php&portal_id=itop-portal )
  2. User gets redirected by an extension to the actual URL: https://demo.combodo.com/simple/pages/exec.php/object/create/UserRequest?exec_module=itop-portal-base&exec_page=index.php&portal_id=itop-portal (non-modal).
  3. Window should not be closed, IMHO?

Isn't the current logic only based on the assumption that the above URL is shown in a modal dialog (while this could be much more versatile, I'm using it outside a modal)?

@Molkobain Molkobain removed their assignment Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Pending contributor update
Development

Successfully merging this pull request may close these issues.

3 participants