PaulNieuwelaar / alertjs

Dialog Builder allows you to create fully customisable dialogs and popups in Dynamics 365.
https://www.magnetismsolutions.com/our-products/alertjs-alert-popup-for-d365
Other
96 stars 38 forks source link

Validation Issue #7

Closed forceworks closed 6 years ago

forceworks commented 6 years ago

Hi Paul,

It seems with V9, alert.js is failing on these unsupported items:

JavaScript - Do not assume that the parent window is the Microsoft Dynamics 365/CRM form | High | ..\WebResources\h2\js | alert.js | 26 JavaScript - Do not assume that the parent window is the Microsoft Dynamics 365/CRM form | High | ..\WebResources\h2\js | alert.js | 55 JavaScript - Do not assume that the parent window is the Microsoft Dynamics 365/CRM form | High | ..\WebResources\h2_\js | alert.js | 59

Is this something you are aware of?

PaulNieuwelaar commented 6 years ago

Hi, I've just tested some of the functions again on V9, in particular the getCrmWindow and getIFrameWindow and they both seem to be returning the correct objects for me. Could you share the code snippet you're using to display the alert? And if you're displaying a web resource, could you share any code that references back to the Alert.js library? Also, at what point does the error occur? Cheers, Paul

forceworks commented 6 years ago

Thanks for replying Paul,

The functionality works. The issue is that Microsoft treats window.parent, document.parent and parent.$ as unsupported code from version 9 on.

Thanks!

Steve Mordue 813-263-7129

From: Paul Nieuwelaar [mailto:notifications@github.com] Sent: Tuesday, November 21, 2017 3:01 PM To: PaulNieuwelaar/alertjs alertjs@noreply.github.com Cc: Steve Mordue Steve@forceworks.com; Author author@noreply.github.com Subject: Re: [PaulNieuwelaar/alertjs] Validation Issue (#7)

Hi, I've just tested some of the functions again on V9, in particular the getCrmWindow and getIFrameWindow and they both seem to be returning the correct objects for me. Could you share the code snippet you're using to display the alert? And if you're displaying a web resource, could you share any code that references back to the Alert.js library? Also, at what point does the error occur? Cheers, Paul

โ€” You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/PaulNieuwelaar/alertjs/issues/7#issuecomment-346143900, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ANE2rOmP4d2_NfqRQ5nA2URNv_9efCR_ks5s4yvvgaJpZM4QmQMs.

PaulNieuwelaar commented 6 years ago

Ah yup, in this case it's only using window.parent to get access to jQuery, and it handles both scenarios of turbo forms on/off. It also appends the alert wrapper directly to the top frame, so it doesn't matter about the form context etc, it will work regardless. This library is still technically unsupported as it manipulates the DOM, but it's done in a way that has very low dependencies on anything CRM, so it "should" continue to work going forward. Cheers for the feedback ๐Ÿ˜ƒ - Paul

amervitz commented 6 years ago

A ramification of the unsupported techniques used is Microsoft isnโ€™t allowing submissions to AppSource with this library included as a dependency. Iโ€™d suggest this caveat be disclosed in the readme.

PaulNieuwelaar commented 6 years ago

Hi Alan, I'm pleased to announce that Alert.js v2.1 has now been approved for App Source, so you should have no problems listing this as a dependency now ๐Ÿ˜ƒ- Paul

amervitz commented 6 years ago

@PaulNieuwelaar that's great news. Were there any changes required (and which commit has the changes?) or are the AppSource certification team now allowing the techniques that are taken? It would be very helpful to reference something if this gets flagged in the future during certification.

PaulNieuwelaar commented 6 years ago

No changes required, so it's just the current version 2.1. Things like "parent." will still get flagged by the automated verification process, but you should be able to reference the Alert.js app source page to show that it's already been approved.