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

[MOB-2858] Ecosify all ecosia urls #764

Merged
merged 4 commits into from
Sep 2, 2024

Conversation

lucaschifino
Copy link

@lucaschifino lucaschifino commented Aug 29, 2024

MOB-2858

Context

A bug was reported by BI where we have duplicated user ids for consecutive in-page SERP queries in verticals other than search.

Approach

Using the new method introduced on https://github.com/ecosia/ios-core/pull/158.

Before merging

  • Merge core PR first and update package.

Checklist

  • I performed some relevant testing on a real device and/or simulator

Copy link

github-actions bot commented Aug 29, 2024

PR Reviewer Guide 🔍

(Review updated until commit 00c06cb)

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Logic Change
The condition for URL modification has changed from checking if the URL is an Ecosia search query to checking if it should be ecosified. Ensure this new condition correctly encompasses all intended scenarios without unintended side effects.

@lucaschifino lucaschifino requested review from rashmi-ecosia and a team August 29, 2024 09:47
Copy link
Member

@d4r1091 d4r1091 left a comment

Choose a reason for hiding this comment

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

All good my side. Added a small suggestion and will await the Core SPM update to Approve!

@lucaschifino lucaschifino marked this pull request as ready for review August 30, 2024 12:53
Copy link

Persistent review updated to latest commit 00c06cb

Copy link

github-actions bot commented Aug 30, 2024

PR Code Suggestions ✨

Latest suggestions up to 00c06cb

CategorySuggestion                                                                                                                                    Score
Maintainability
Simplify conditional logic using a guard statement

Consider using a guard statement to simplify the logic and reduce nesting. This
makes the code easier to read and maintain.

Client/Frontend/Toolbar+URLBar/URLBarView.swift [242-243]

-if updatedUrl?.shouldEcosify() ?? false {
-    updatedUrl = newURL?.ecosified(isIncognitoEnabled: isPrivate)
-}
+guard updatedUrl?.shouldEcosify() ?? false else { return }
+updatedUrl = newURL?.ecosified(isIncognitoEnabled: isPrivate)
 
Suggestion importance[1-10]: 7

Why: The suggestion to use a guard statement improves code readability and maintainability by reducing nesting. However, it slightly changes the logic flow by introducing an early return, which should be carefully considered in the context of the surrounding code.

7

Previous suggestions

Suggestions up to commit ed8c6ad
CategorySuggestion                                                                                                                                    Score
Maintainability
Simplify conditional logic using a guard statement

Consider using a guard statement to simplify the logic and reduce nesting. This will
make the code easier to read and maintain.

Client/Frontend/Toolbar+URLBar/URLBarView.swift [242-243]

-if updatedUrl?.shouldEcosify() ?? false {
-    updatedUrl = newURL?.ecosified(isIncognitoEnabled: isPrivate)
-}
+guard updatedUrl?.shouldEcosify() ?? false else { return }
+updatedUrl = newURL?.ecosified(isIncognitoEnabled: isPrivate)
 
Suggestion importance[1-10]: 7

Why: The suggestion to use a guard statement is valid as it simplifies the conditional logic and reduces nesting, which can improve code readability and maintainability. However, it is not a critical change, so it receives a moderate score. The existing code is correct, and the improved code accurately reflects the suggestion. The context of the code around the suggestion is also considered.

7

Copy link
Member

@d4r1091 d4r1091 left a comment

Choose a reason for hiding this comment

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

🚀

@lucaschifino lucaschifino merged commit e0c61b4 into main Sep 2, 2024
2 checks passed
@lucaschifino lucaschifino deleted the ls-mob-2858-ecosify-all-ecosia-urls branch September 2, 2024 06:43
Sign up for free to 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