Skip to content

Bump jsoup to 1.15.3 - #1089

Merged
wzieba merged 3 commits into
trunkfrom
bump_jsoup_1_15_3
Aug 8, 2024
Merged

Bump jsoup to 1.15.3#1089
wzieba merged 3 commits into
trunkfrom
bump_jsoup_1_15_3

Conversation

@wzieba

@wziebawzieba commented Aug 7, 2024

Copy link
Copy Markdown
Contributor

Version without reported security vulnerabilities

To address https://github.com/wordpress-mobile/WordPress-Android/security/dependabot/91

Test

Smoke test the "code editor" (last button) in AztecDemo app.

Version without reported security vulnerabilities
It was `Element` when jsoup was `1.11.3`
@wzieba
wzieba requested a review from ParaskP7August 7, 2024 14:50
@wzieba
wzieba marked this pull request as ready for review August 7, 2024 14:50

@ParaskP7ParaskP7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👋 @wzieba !

I have reviewed and tested this PR as well, everything works as expected, good job! 🌟


I have just a minor (🔍) comment for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself (after fixing the UI test issue).

.filter { !it.hasText() && it.tagName() == "span" && it.childNodes().size == 0 }
.forEach { it.remove() }
val select: Elements = doc.select("*")
select.filter { element: Element ->

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor (🔍): Why exposing this select local variable if not using it, maybe you had something in mind? 🤔 Consider, reverting it to it's previous form: doc.select("*").filter { element: Element -> ... }

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I thought it'll be more readable, but maybe it's not the case. Inlined in: 5bee1c9

@wzieba
wzieba enabled auto-merge August 8, 2024 13:33
@wzieba
wzieba disabled auto-merge August 8, 2024 13:33
@wzieba
wzieba enabled auto-merge August 8, 2024 13:33
@wzieba
wzieba merged commit bdd9858 into trunkAug 8, 2024
@wzieba
wzieba deleted the bump_jsoup_1_15_3 branch August 8, 2024 13:41
@ParaskP7

Copy link
Copy Markdown
Contributor

(after fixing the UI test issue)

Btw, did we manage to fix the UI test issue @wzieba ? 🤔

@wzieba

Copy link
Copy Markdown
ContributorAuthor

The test happened to be just flaky. I run it locally a few times, it always passed. On CI, I rerun it twice and passed each time.

@ParaskP7

Copy link
Copy Markdown
Contributor

Oh great, thanks for the confirmation on that! 🙇 😌

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@wzieba@ParaskP7