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.

Sign up for GitHub

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

Jump to bottom

upgrade jquery-ui version #5116

Merged
merged 11 commits into from Aug 18, 2022
Merged

upgrade jquery-ui version #5116

merged 11 commits into from Aug 18, 2022

Conversation

armaganpekatik
Copy link
Contributor

@armaganpekatik armaganpekatik commented May 9, 2022 •
edited by valadas

Fixes #5115
Closes #4816

Summary

DNN is using an old version of jquery-ui 1.12.1
Upgrade jquery-ui to newer version from 1.12.1 to latest 1.13.1

mitchelsellers
mitchelsellers reviewed May 9, 2022
View reviewed changes
Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

Has this been validated to not impact the functionality of core DNN Features? I believe the only major function is the modal dialogs (Login, Module Settings, etc).

@armaganpekatik
Copy link
Contributor Author

Has this been validated to not impact the functionality of core DNN Features? I believe the only major function is the modal dialogs (Login, Module Settings, etc).

No it hasn't. How to proceed these steps?

@mitchelsellers
Copy link
Contributor

At a minimum after updating a package such as this we need to ensure that the standard flows that are utilizing this functionality are still operational without error.

This would involve at a minimum the setup of a clean instance using the artifacts from this build, and then validating that the items dependent upon this functionality are still operational. I believe, at a minimum, the areas that use this are the Login popup and Module Settings. But there might be more.

@armaganpekatik
Copy link
Contributor Author

At a minimum after updating a package such as this we need to ensure that the standard flows that are utilizing this functionality are still operational without error.

This would involve at a minimum the setup of a clean instance using the artifacts from this build, and then validating that the items dependent upon this functionality are still operational. I believe, at a minimum, the areas that use this are the Login popup and Module Settings. But there might be more.

My question is how to prove this as working. Is there any pass QBs? Or do you want me to just test it as Ad-Hoc?

@mitchelsellers
Copy link
Contributor

We do not have any automated UI testing for this at this time, validation that pop-ups appear, without console errors is the best first-level validation at this time.

@armaganpekatik
Copy link
Contributor Author

We do not have any automated UI testing for this at this time, validation that pop-ups appear, without console errors is the best first-level validation at this time.

Who shall do this test? Is it ok if I do this as ad-hoc and say done? What kind of proof shall I bring?

@mitchelsellers
Copy link
Contributor

Yes, totally fine for you.

Just before we take the time to also validate (which we will) we want to know that it has been tested.

Simply acknowledgment that you have tested is enough here

@armaganpekatik
Copy link
Contributor Author

Installer is taken from https://dev.azure.com/dotnet/DNN/_build/results?buildId=71787&view=results

1.13.1 Jquery-UI path is shown in ss

Login screen with console
image

Extension settings with console
image

Tabs, sub-tabs and different layouts
image

Note: This Uncaught TypeError: c is not a function error is also there in the current version. It is not new.

valadas
valadas approved these changes May 9, 2022
View reviewed changes
Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Looks about right to me. Thanks!

Now if we would have a v9.10.x release, would we put this in it or should we target 9.11.x ?

@bdukes
Copy link
Contributor

bdukes commented May 10, 2022

I believe there are more places that need to be updated to be consistently using the new version:

@bdukes bdukes added this to the 9.11.0 milestone May 10, 2022
@armaganpekatik
Copy link
Contributor Author

I believe there are more places that need to be updated to be consistently using the new version:

I am not sure if nothing goes wrong if we update these versions. Some of them look like component specific to me. This can be done through this #5114

@mitchelsellers
Copy link
Contributor

We need to have all versions of jQuery UI standardized before we can merge this pull request, as we do not want to introduce diverging versions.

@bdukes
Copy link
Contributor

bdukes commented Jul 15, 2022

1.13.2 has been released

@bdukes bdukes added Type: Maintenance javascript Pull requests that update Javascript code labels Jul 19, 2022
@nick-stepanenko
Copy link

Hi, team, @bdukes , @mitchelsellers , @valadas !

I've seen different issues related to JQuery/Jquery-UI upgrade:
#4816 (Upgrade to latest jQuery UI)
#5114 (Standardize jQuery Usage to Current Version)
#5115 (Update Jquery-UI version from 1.12.1 to latest)
and this PR: #5116

  • valuable comments from the issues above:
    PR 5116: We need to have all versions of jQuery UI standardized before we can merge this pull request, as we do not want to introduce diverging versions.
    Issue 5114: We need to ensure that all versions, including those loaded from a CDN are secure and up to latest versions. Ideally using the standardized registration processes for jQuery.

  1. Could you please confirm, that Jquery and JQuery-UI are separate upgrades (e.g. release them in different DNN versions, so we can focus on JQuery-UI upgrade only and release it first)?

  2. What do you mean by "standardized registration processes for jQuery"(I believe this should be also applied to JQuery-UI)?

Per this comment #5116 (comment) I can see 2 patterns: hard-coded urls(external components) and embedded Jquey/JQuery-UI files into DNN codebase.
I guess we can have:

  1. Global config file to store external library URLs as parameters, so we can manage external JQuery-UI dependencies in one place
  2. We could choose the same approach for local files, e.g. keep all the used versions locally and use global 'selector' parameter to define which version is used.
  3. If you already have standards that I'm not aware of - could you please share them?

Best regards, Nick

@mitchelsellers
Copy link
Contributor

#4816 & #5115 Are duplicate issues, and #4816 is the one that actually did the upgrade, as such #5116 (this item) can be closed as it is duplicate of the efforts from the other PR.

However, as noted by @bdukes there are other places that we need to standardize as part of the larger, OTHER jQUery issue #5114.

Given where we are today, I would love to be standardized 100% on using the DNN way to include this, but I don't believe it is fully possible. (CKEditor for example loads in a page, not a DNN context to the DNN way to load isn't going to work)

We need to just make sure that we get everything updated, and to the SAME version. If we can centralize the location/version that would be great, but might be a bit out of scope for the current time where we just need to get the upgrade done.

@nick-stepanenko
Copy link

Hi, @mitchelsellers !
Thanks for your response.

I'm a bit confused: #4816 does not have any linked PRs, except this one (linked to the issue by @valadas in this comment: #4816 (comment)).

So, I consider this PR is the only one that contains JQuery-UI updates (although we still need to update all the JQuery-UI occurrences + it's better to update to the most actual version: 1.13.2) and fixes #4816

Given that we are going to fix all the JQuery-UI occurrences, unsolved larger JQuery issue #5114 would not be a blocker to merge this PR into the next upcoming release.

Please, confirm my understanding.

@mitchelsellers
Copy link
Contributor

@nick-stepanenko wow, sorry about that, following links I got turned around a bit.

You are correct, I would propose the following.

@mitchelsellers
Copy link
Contributor

@nick-stepanenko Are you planning any effort on this issue?

@hermetheuscoffee
Copy link

@nick-stepanenko since this issue was created, JQuery UI has released a newer build (1.13.2). Is it possible for this build to be used for this issue instead of 1.13.1? Thank you!

@bdukes bdukes linked an issue Aug 18, 2022 that may be closed by this pull request
Standardize jQuery Usage to Current Version #5114
Closed
@bdukes
Copy link
Contributor

bdukes commented Aug 18, 2022

@dnnsoftware/approvers This PR is ready for review.

I also upgraded jQuery Migrate from 3.2.0 to 3.4.0 and a couple of places we were still using jQuery 3.2.1 or 3.4.1 instead of 3.5.1

mitchelsellers
mitchelsellers approved these changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

@bdukes You are awesome!

@mitchelsellers mitchelsellers merged commit 68d0f0a into dnnsoftware:develop Aug 18, 2022
@bdukes bdukes mentioned this pull request Aug 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mitchelsellers mitchelsellers mitchelsellers approved these changes

@valadas valadas valadas approved these changes

Assignees
No one assigned
Labels
javascript Pull requests that update Javascript code Type: Maintenance
Projects
None yet
Milestone
9.11.0
6 participants
@armaganpekatik @mitchelsellers @bdukes @nick-stepanenko @hermetheuscoffee @valadas