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

Bug 1897319 - Stores icons for light/dark theme for Yelp suggestion #6248

Closed
wants to merge 2 commits into from
Closed

Bug 1897319 - Stores icons for light/dark theme for Yelp suggestion #6248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5b133a
Supports icons dependent on theme
dadaa May 16, 2024
1907a14
Introduce suggestion icon struct
dadaa May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions components/suggest/src/db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use crate::{
DownloadedWikipediaSuggestion, Record, SuggestRecordId,
},
schema::{clear_database, SuggestConnectionInitializer},
suggestion::{cook_raw_suggestion_url, AmpSuggestionType, Suggestion},
suggestion::{cook_raw_suggestion_url, AmpSuggestionType, Suggestion, SuggestionIcon},
Result, SuggestionQuery,
};

Expand Down Expand Up @@ -299,7 +299,7 @@ impl<'a> SuggestDao<'a> {
amp.iab_category,
amp.impression_url,
amp.click_url,
i.data AS icon,
i.data AS icon_data,
i.mimetype AS icon_mimetype
FROM
amp_custom_details amp
Expand All @@ -315,6 +315,12 @@ impl<'a> SuggestDao<'a> {
let cooked_url = cook_raw_suggestion_url(&raw_url);
let raw_click_url = row.get::<_, String>("click_url")?;
let cooked_click_url = cook_raw_suggestion_url(&raw_click_url);
let icon_data = row.get::<_, Option<_>>("icon_data")?;
let icon_mime_type = row.get::<_, Option<_>>("icon_mimetype")?;
let icon = icon_data.map(|data| SuggestionIcon {
data,
mime_type: icon_mime_type.unwrap_or_default(),
});

Ok(Suggestion::Amp {
block_id: row.get("block_id")?,
Expand All @@ -325,8 +331,7 @@ impl<'a> SuggestDao<'a> {
raw_url,
full_keyword: full_keyword_from_db
.unwrap_or_else(|| full_keyword(keyword_lowercased, &keywords)),
icon: row.get("icon")?,
icon_mimetype: row.get("icon_mimetype")?,
icon,
impression_url: row.get("impression_url")?,
click_url: cooked_click_url,
raw_click_url,
Expand Down Expand Up @@ -378,7 +383,7 @@ impl<'a> SuggestDao<'a> {
},
|row| row.get(0),
)?;
let (icon, icon_mimetype) = self
let icon = self
.conn
.try_query_row(
"SELECT i.data, i.mimetype
Expand All @@ -390,21 +395,20 @@ impl<'a> SuggestDao<'a> {
":suggestion_id": suggestion_id
},
|row| -> Result<_> {
Ok((
row.get::<_, Option<Vec<u8>>>(0)?,
row.get::<_, Option<String>>(1)?,
))
Ok(Some(SuggestionIcon {
data: row.get::<_, Vec<u8>>(0)?,
mime_type: row.get::<_, String>(1)?,
}))
},
true,
)?
.unwrap_or((None, None));
.unwrap_or(None);

Ok(Suggestion::Wikipedia {
title,
url: raw_url,
full_keyword: full_keyword(keyword_lowercased, &keywords),
icon,
icon_mimetype,
})
},
)?;
Expand Down Expand Up @@ -986,7 +990,7 @@ impl<'a> SuggestDao<'a> {
}

/// Inserts or replaces an icon for a suggestion into the database.
pub fn put_icon(&mut self, icon_id: &str, data: &[u8], mimetype: &str) -> Result<()> {
pub fn put_icon(&mut self, icon_id: &str, data: &[u8], mime_type: &str) -> Result<()> {
self.conn.execute(
"INSERT OR REPLACE INTO icons(
id,
Expand All @@ -1001,7 +1005,7 @@ impl<'a> SuggestDao<'a> {
named_params! {
":id": icon_id,
":data": data,
":mimetype": mimetype,
":mimetype": mime_type,
},
)?;
Ok(())
Expand Down
2 changes: 1 addition & 1 deletion components/suggest/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub use error::SuggestApiError;
pub use provider::SuggestionProvider;
pub use query::SuggestionQuery;
pub use store::{InterruptKind, SuggestIngestionConstraints, SuggestStore, SuggestStoreBuilder};
pub use suggestion::{raw_suggestion_url_matches, Suggestion};
pub use suggestion::{raw_suggestion_url_matches, Suggestion, SuggestionIcon};

pub(crate) type Result<T> = std::result::Result<T, error::Error>;
pub type SuggestApiResult<T> = std::result::Result<T, error::SuggestApiError>;
Expand Down
6 changes: 4 additions & 2 deletions components/suggest/src/rs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,8 +461,10 @@ pub(crate) struct DownloadedYelpSuggestion {
pub location_signs: Vec<DownloadedYelpLocationSign>,
#[serde(rename = "yelpModifiers")]
pub yelp_modifiers: Vec<String>,
#[serde(rename = "icon")]
pub icon_id: String,
#[serde(rename = "iconLightTheme")]
pub icon_light_theme_id: String,
#[serde(rename = "iconDarkTheme")]
pub icon_dark_theme_id: String,
pub score: f64,
}

Expand Down
60 changes: 57 additions & 3 deletions components/suggest/src/schema.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to bump the schema VERSION const and add a migration step to upgrade_from(). See the comment above VERSION.

Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use sql_support::open_database::{self, ConnectionInitializer};
/// [`SuggestConnectionInitializer::upgrade_from`].
/// a. If suggestions should be re-ingested after the migration, call `clear_database()` inside
/// the migration.
pub const VERSION: u32 = 19;
pub const VERSION: u32 = 20;

/// The current Suggest database schema.
pub const SQL: &str = "
Expand Down Expand Up @@ -113,9 +113,11 @@ CREATE TABLE yelp_location_signs(
) WITHOUT ROWID;

CREATE TABLE yelp_custom_details(
icon_id TEXT PRIMARY KEY,
icon_light_theme_id TEXT NOT NULL,
icon_dark_theme_id TEXT NOT NULL,
Comment on lines +116 to +117
Copy link
Contributor

Choose a reason for hiding this comment

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

Storing both light and dark icons in the same row is OK with me, and also I just wanted to mention another way to do it would be to have one icon per row plus an "icon type" column. If we need to support any more types of icons in the future -- and I don't know why we would! -- I think we should switch to that approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, if we do not know how many icons we should handle, then yeah, should introduce the mechanism.

score REAL NOT NULL,
record_id TEXT NOT NULL
record_id TEXT NOT NULL,
PRIMARY KEY (icon_light_theme_id, icon_dark_theme_id)
) WITHOUT ROWID;

CREATE TABLE mdn_custom_details(
Expand Down Expand Up @@ -193,6 +195,34 @@ CREATE TABLE IF NOT EXISTS dismissed_suggestions (
)?;
Ok(())
}
19 => {
let result = tx.query_row_and_then(
"SELECT icon_id FROM yelp_custom_details LIMIT 1",
(),
|row| row.get::<_, String>(0),
);

if let Ok(_) = result {
// As still old table, we chnage to new.
tx.execute_batch(
"
ALTER TABLE yelp_custom_details RENAME TO old_yelp_custom_details;
CREATE TABLE yelp_custom_details(
icon_light_theme_id TEXT NOT NULL,
icon_dark_theme_id TEXT NOT NULL,
score REAL NOT NULL,
record_id TEXT NOT NULL,
PRIMARY KEY (icon_light_theme_id, icon_dark_theme_id)
) WITHOUT ROWID;
INSERT INTO yelp_custom_details (icon_light_theme_id, icon_dark_theme_id, score, record_id)
SELECT icon_id, icon_id, score, record_id FROM old_yelp_custom_details;
DROP TABLE old_yelp_custom_details;
",
)?;
}

Ok(())
}
_ => Err(open_database::Error::IncompatibleVersion(version)),
}
}
Expand Down Expand Up @@ -320,6 +350,8 @@ CREATE TABLE yelp_custom_details(
record_id TEXT NOT NULL
) WITHOUT ROWID;

INSERT INTO yelp_custom_details(icon_id, score, record_id) VALUES ('yelp-icon', 0.5, 'yelp-record');

CREATE TABLE mdn_custom_details(
suggestion_id INTEGER PRIMARY KEY,
description TEXT NOT NULL,
Expand All @@ -338,5 +370,27 @@ PRAGMA user_version=16;
let db_file = MigratedDatabaseFile::new(SuggestConnectionInitializer, V16_SCHEMA);
db_file.run_all_upgrades();
db_file.assert_schema_matches_new_database();

// Test wether yelp_custom_details data inherits to new table.
let connection = db_file.open();
let result: Result<(String, String, f64, String), crate::error::Error> = connection.query_row_and_then(
"SELECT icon_light_theme_id, icon_dark_theme_id, score, record_id FROM yelp_custom_details LIMIT 1",
(),
|row| Ok((
row.get::<_, String>(0)?,
row.get::<_, String>(1)?,
row.get::<_, f64>(2)?,
row.get::<_, String>(3)?
))
);

if let Ok((icon_light_theme_id, icon_dark_theme_id, score, record_id)) = result {
assert_eq!(icon_light_theme_id, "yelp-icon");
assert_eq!(icon_dark_theme_id, "yelp-icon");
assert_eq!(score, 0.5);
assert_eq!(record_id, "yelp-record");
} else {
assert!(false);
}
}
}
Loading