-
Notifications
You must be signed in to change notification settings - Fork 16
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
type(feat): Implement Bookmark Classes and Tests #327
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# This Source Code Form is subject to the terms of the Mozilla Public | ||
# License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
# You can obtain one at http://mozilla.org/MPL/2.0/. | ||
"""Contains the bookmarks interaction API and supporting files.""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,218 @@ | ||
# This Source Code Form is subject to the terms of the Mozilla Public | ||
# License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
# You can obtain one at http://mozilla.org/MPL/2.0/. | ||
"""Contains classes for handling Firefox bookmarks.""" | ||
|
||
from selenium.webdriver.common.by import By | ||
from selenium.common.exceptions import NoSuchElementException | ||
from selenium.webdriver.remote.webelement import WebElement | ||
from selenium.webdriver.common.keys import Keys | ||
from foxpuppet.windows.browser.navbar import NavBar | ||
from typing import TYPE_CHECKING, Optional, TypedDict, List | ||
|
||
|
||
class BookmarkData(TypedDict): | ||
"""Bookmark properties.""" | ||
|
||
name: str | ||
url: str | ||
tags: Optional[List[str]] | ||
keyword: Optional[str] | ||
|
||
|
||
class BasicBookmark(NavBar): | ||
"""Handles basic bookmark operations.""" | ||
|
||
if TYPE_CHECKING: | ||
from foxpuppet.windows.browser.window import BrowserWindow | ||
|
||
@staticmethod | ||
def create(window: "BrowserWindow", root: WebElement) -> Optional["BasicBookmark"]: | ||
"""Create a bookmark object. | ||
|
||
Args: | ||
window (:py:class:`BrowserWindow`): Window object this bookmark appears in | ||
root (:py:class:`~selenium.webdriver.remote.webelement.WebElement`): WebDriver element object for bookmark | ||
|
||
Returns: | ||
:py:class:`BaseBookmark`: Bookmark instance or None | ||
""" | ||
with window.selenium.context(window.selenium.CONTEXT_CHROME): | ||
try: | ||
return BasicBookmark(window, root) | ||
except NoSuchElementException: | ||
return None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am curious as to why you'd like to return None and not throw the error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be less verbose. But, now again, it could confusing and unclear. I will just correct that. |
||
|
||
@property | ||
def is_bookmarked(self) -> bool: | ||
"""Checks if the current page is bookmarked. | ||
|
||
Returns: | ||
bool: True if the page is bookmarked, False otherwise. | ||
""" | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
star_button_image = self.find_element(BookmarkLocators.STAR_BUTTON_IMAGE) | ||
return star_button_image.get_attribute("starred") == "true" | ||
|
||
def add(self) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the difference with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should probably use a flag instead and combine both into one method so it isn't confusing |
||
"""Add a Bookmark using the star button.""" | ||
self.click_element(BookmarkLocators.STAR_BUTTON) | ||
self.click_element(BookmarkLocators.FOLDER_MENU) | ||
self.click_element(BookmarkLocators.OTHER_BOOKMARKS_STAR) | ||
self.click_element(BookmarkLocators.SAVE_BUTTON) | ||
|
||
def retrieve_bookmark(self, label: str) -> bool: | ||
""" | ||
Check if a bookmark with the given label exists. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function name reads as if this will return a bookmark but it seems to be more of a check. Can you update the name to reflect that? |
||
Args: | ||
label (str): The name of the bookmark to search for. | ||
""" | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
self.open_bookmark_menu( | ||
locator_panel=BookmarkLocators.PANEL_MENU, | ||
locator_bookmark=BookmarkLocators.PANEL_BOOKMARK_MENU, | ||
) | ||
panel_bookmarks = self.find_element(BookmarkLocators.PANEL_BOOKMARK_TOOLBAR) | ||
if panel_bookmarks is not None: | ||
menu_items = panel_bookmarks.find_elements( | ||
By.CSS_SELECTOR, "toolbarbutton.bookmark-item" | ||
) | ||
if menu_items is not None: | ||
for item in menu_items: | ||
item_label = item.get_attribute("label") | ||
if item_label and label.lower() in item_label.lower(): | ||
return True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could probably be done with a list comprehension. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought to go with a generator expression here which might better fit, since the intention here is not to create a list, but to evaluate a condition. |
||
return False | ||
|
||
def delete(self) -> None: | ||
"""Delete a bookmark using the star button.""" | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
star_button_image = self.find_element(BookmarkLocators.STAR_BUTTON_IMAGE) | ||
if star_button_image and star_button_image.get_attribute("starred") == "true": | ||
self.click_element(BookmarkLocators.STAR_BUTTON) | ||
self.click_element(BookmarkLocators.REMOVE_BUTTON) | ||
|
||
|
||
class AdvancedBookmark(BasicBookmark): | ||
"""Handles advanced bookmark operations.""" | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's an "Advanced Bookmark" exactly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, so in Desktop Smoke test, a similar term is used so I decided to adopt it in FoxPuppet. Basically, an advanced bookmark is a more detailed way of adding a bookmark, which involves There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay let me ask Ben C for clarifications There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ben-c-at-moz Can you explain what your "Advanced Bookmark" action does exactly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is maybe not the best name--the function here is "advanced" only in that it adds a bookmark through the "Other bookmarks" option in the Library window. The bookmark itself should be normal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I agree, the name is a little confusing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But I'm fine if we just want to keep using it for more detailed bookmark operations, as long as we explain it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we just call it |
||
if TYPE_CHECKING: | ||
from foxpuppet.windows.browser.window import BrowserWindow | ||
|
||
@staticmethod | ||
def create(window: "BrowserWindow", root: WebElement) -> Optional["AdvancedBookmark"]: | ||
"""Create an advanced bookmark object. | ||
|
||
Args: | ||
window (:py:class:`BrowserWindow`): The window object where the bookmark appears. | ||
root (:py:class:`~selenium.webdriver.remote.webelement.WebElement`): WebElement for the bookmark panel. | ||
|
||
Returns: | ||
:py:class:`AdvancedBookmark`: An instance of AdvancedBookmark if successful, otherwise None. | ||
""" | ||
with window.selenium.context(window.selenium.CONTEXT_CHROME): | ||
try: | ||
return AdvancedBookmark(window, root) | ||
except NoSuchElementException: | ||
return None | ||
|
||
@property | ||
def is_bookmarked(self) -> bool: | ||
"""Checks if the current page is bookmarked. | ||
|
||
Returns: | ||
bool: True if the page is bookmarked, False otherwise. | ||
""" | ||
current_page_title = self.selenium.title | ||
self.open_main_menu( | ||
locator_toolbar=BookmarkLocators.NAVIGATOR_TOOLBOX, | ||
locator_menu_bar=BookmarkLocators.MENU_BAR, | ||
) | ||
bookmark_menu = self.find_element(BookmarkLocators.MAIN_MENU_BOOKMARK) | ||
if bookmark_menu is not None: | ||
self.click_element(BookmarkLocators.MAIN_MENU_BOOKMARK) | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
menu_items = bookmark_menu.find_elements( | ||
By.CSS_SELECTOR, "menuitem.bookmark-item" | ||
) | ||
for item in menu_items: | ||
item_label = item.get_attribute("label") | ||
if item_label and item_label.lower() in current_page_title.lower(): | ||
return True | ||
return False | ||
|
||
def add_bookmark(self, bookmark_data: BookmarkData) -> None: | ||
"""Add a Bookmark using the main bookmark menu.""" | ||
self.open_main_menu( | ||
locator_toolbar=BookmarkLocators.NAVIGATOR_TOOLBOX, | ||
locator_menu_bar=BookmarkLocators.MENU_BAR, | ||
) | ||
self.click_element(BookmarkLocators.MAIN_MENU_BOOKMARK) | ||
self.context_click(BookmarkLocators.MANAGE_BOOKMARKS) | ||
self.click_element(BookmarkLocators.ADD_BOOKMARK) | ||
self.switch_to_frame(BookmarkLocators.ADD_BOOKMARK_FRAME) | ||
|
||
if bookmark_data["name"]: | ||
self.actions.send_keys(bookmark_data["name"]).perform() | ||
self.actions.send_keys(Keys.TAB).perform() | ||
|
||
if bookmark_data["url"]: | ||
self.actions.send_keys(bookmark_data["url"] + Keys.TAB).perform() | ||
|
||
tags = bookmark_data["tags"] | ||
if tags is not None: | ||
for tag in tags: | ||
self.actions.send_keys(tag).perform() | ||
self.actions.send_keys(",").perform() | ||
self.actions.send_keys(Keys.TAB).perform() | ||
|
||
if bookmark_data.get("keyword"): | ||
keyword = ( | ||
bookmark_data["keyword"] if bookmark_data["keyword"] is not None else "" | ||
) | ||
self.actions.send_keys(keyword + Keys.TAB).perform() | ||
|
||
self.actions.send_keys(Keys.TAB, Keys.TAB, Keys.TAB, Keys.ENTER).perform() | ||
self.actions.send_keys(Keys.TAB, Keys.ENTER).perform() | ||
self.switch_to_default_context() | ||
|
||
def delete_bookmark(self, label: str) -> bool: | ||
"""Delete a bookmark using the main bookmark menu.""" | ||
self.open_main_menu( | ||
locator_toolbar=BookmarkLocators.NAVIGATOR_TOOLBOX, | ||
locator_menu_bar=BookmarkLocators.MENU_BAR, | ||
) | ||
bookmark_menu = self.find_element(BookmarkLocators.MAIN_MENU_BOOKMARK) | ||
self.click_element(BookmarkLocators.MAIN_MENU_BOOKMARK) | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
menu_item = bookmark_menu.find_element( | ||
By.CSS_SELECTOR, f"menuitem.bookmark-item[label='{label}']" | ||
) | ||
self.actions.context_click(menu_item).perform() | ||
self.click_element(BookmarkLocators.DELETE_MENU_ITEM) | ||
return True | ||
|
||
|
||
class BookmarkLocators: | ||
ADD_BOOKMARK = (By.ID, "placesContext_new:bookmark") | ||
ADD_BOOKMARK_FRAME = (By.CSS_SELECTOR, "browser[class='dialogFrame']") | ||
BOOKMARK_PROPERTIES_DIALOG = (By.ID, "bookmarkproperties") | ||
DELETE_MENU_ITEM = (By.ID, "placesContext_deleteBookmark") | ||
FOLDER_MENU = (By.ID, "editBMPanel_folderMenuList") | ||
MAIN_MENU_BOOKMARK = (By.ID, "bookmarksMenu") | ||
MANAGE_BOOKMARKS = (By.ID, "bookmarksShowAll") | ||
MENU_BAR = (By.ID, "toggle_toolbar-menubar") | ||
NAME_FIELD = (By.ID, "editBMPanel_namePicker") | ||
NAVIGATOR_TOOLBOX = (By.ID, "navigator-toolbox") | ||
OTHER_BOOKMARKS = (By.ID, "OtherBookmarks") | ||
OTHER_BOOKMARKS_STAR = (By.ID, "editBMPanel_unfiledRootItem") | ||
PANEL_BOOKMARK_MENU = (By.ID, "appMenu-bookmarks-button") | ||
PANEL_BOOKMARK_TOOLBAR = (By.ID, "panelMenu_bookmarksMenu") | ||
PANEL_MENU = (By.ID, "PanelUI-menu-button") | ||
REMOVE_BUTTON = (By.ID, "editBookmarkPanelRemoveButton") | ||
SAVE_BOOKMARK = (By.CSS_SELECTOR, 'button[dlgtype="accept"][label="Save"]') | ||
SAVE_BUTTON = (By.ID, "editBookmarkPanelDoneButton") | ||
STAR_BUTTON = (By.ID, "star-button-box") | ||
STAR_BUTTON_IMAGE = (By.ID, "star-button") | ||
TOOLBAR_CONTEXT_MENU = (By.ID, "toolbar-context-menu") |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,13 @@ | |
"""Creates Navbar object to interact with Firefox Navigation Bar.""" | ||
|
||
from selenium.webdriver.common.by import By | ||
|
||
from selenium.webdriver.remote.webelement import WebElement | ||
from selenium.webdriver.remote.webdriver import WebDriver | ||
from selenium.webdriver.common.action_chains import ActionChains | ||
from selenium.common.exceptions import NoSuchElementException | ||
from foxpuppet.region import Region | ||
from typing import Tuple, Optional | ||
from foxpuppet.windows.base import BaseWindow | ||
|
||
|
||
class NavBar(Region): | ||
|
@@ -20,7 +25,14 @@ class NavBar(Region): | |
|
||
""" | ||
|
||
_tracking_protection_shield_locator = (By.ID, "tracking-protection-icon-box") | ||
_tracking_protection_shield_locator: Tuple[str, str] = ( | ||
By.ID, | ||
"tracking-protection-icon-box", | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not |
||
|
||
def __init__(self, window: BaseWindow, root: WebElement) -> None: | ||
super().__init__(window, root) | ||
self.actions: ActionChains = ActionChains(self.selenium) | ||
|
||
@property | ||
def is_tracking_shield_displayed(self) -> bool: | ||
|
@@ -36,3 +48,67 @@ def is_tracking_shield_displayed(self) -> bool: | |
return el.get_attribute("active") is not None | ||
el = self.root.find_element(By.ID, "tracking-protection-icon") | ||
return bool(el.get_attribute("state")) | ||
|
||
def click_element(self, locator: Tuple[str, str]) -> None: | ||
"""Click on an element by its locator.""" | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
self.find_element(locator).click() | ||
|
||
def find_element(self, locator: Tuple[str, str]) -> WebElement: | ||
"""Find and return a web element by its locator.""" | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
try: | ||
element = self.root.find_element(*locator) | ||
return element | ||
except Exception as e: | ||
raise NoSuchElementException( | ||
f"Error locating element with locator {locator}: {e}" | ||
) | ||
|
||
def context_click(self, locator: Tuple[str, str]) -> None: | ||
""" | ||
Perform a right-click (context-click) on an element. | ||
|
||
Args: | ||
locator (tuple): Locator for the element to right-click. | ||
""" | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
element = self.find_element(locator) | ||
self.actions.context_click(element).perform() | ||
|
||
def switch_to_frame(self, locator: Tuple[str, str]) -> None: | ||
""" | ||
Switch to an iFrame using its locator | ||
|
||
Args: | ||
locator (tuple): Locator for the iFrame elemeent. | ||
""" | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
frame_element = self.find_element(locator) | ||
if frame_element is not None: | ||
self.selenium.switch_to.frame(frame_element) | ||
else: | ||
raise NoSuchElementException(f"iFrame with locator {locator} not found.") | ||
|
||
def switch_to_default_context(self) -> None: | ||
"""Switch back to default context.""" | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
self.selenium.switch_to.default_content() | ||
|
||
def open_main_menu( | ||
self, locator_toolbar: Tuple[str, str], locator_menu_bar: Tuple[str, str] | ||
) -> None: | ||
"""Activate main menu bar""" | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
self.context_click(locator_toolbar) | ||
self.click_element(locator_menu_bar) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems out of scope for this PR no? Maybe I am missing something There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These methods are used in Bookmarks. I added them here because the issue stated to extend the NavBar with Bookmark functionalities. But then, along the way, I realized Bookmarks doesn't exactly fit with the nav-bar element. While some bookmark web elements exist in the nav bar, others are scattered across different parts of the dom. I could move them all to Bookmark itself if that makes things clearer.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well some of these just seem like generic actions, like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will you suggest I move them to a class higher up the hierarchy? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I moved all the generic actions into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these should be removed if they aren't specifically related to bookmark functionality. We can add these later if need be. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, these methods are functionalities I reused a lot in the Bookmark functionality. So instead of repeating them, I just factored them out into those methods. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand but a lot of them are doing what selenium has built in. Selenium has a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @b4handjr I have refactored and removed all of them. It is now ready for another review. |
||
def open_bookmark_menu( | ||
self, locator_panel: Tuple[str, str], locator_bookmark: Tuple[str, str] | ||
) -> None: | ||
""" | ||
Opens the Bookmarks menu in Panel UI | ||
""" | ||
with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
self.click_element(locator_panel) | ||
self.click_element(locator_bookmark) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏾