Add theme field in Account to represent user's screen color theme
#383
Conversation
WalkthroughThis pull request introduces a new optional Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #383 +/- ##
==========================================
+ Coverage 67.72% 67.81% +0.09%
==========================================
Files 95 95
Lines 19897 20050 +153
==========================================
+ Hits 13475 13597 +122
- Misses 6422 6453 +31 ☔ View full report in Codecov by Sentry. |
div-seungha
force-pushed
the
div/add-dark-mode
branch
from
4f4eb50 to
c15e110
Compare
Cargo.toml
Outdated
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "review-database" | |||
| version = "0.33.0" | |||
| version = "0.33.1-alpha" | |||
There was a problem hiding this comment.
It should be 0.33.1-alpha.1
There was a problem hiding this comment.
I have just modified this line.
div-seungha
force-pushed
the
div/add-dark-mode
branch
from
c15e110 to
dbf1b44
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/migration.rs (1)
3755-3856: Consider adding more test cases for comprehensive coverage.While the current test verifies basic migration functionality, consider adding test cases for:
- Multiple accounts to verify batch migration
- Edge cases like accounts with all optional fields populated
- Error handling scenarios
Example additional test case:
#[test] fn migrate_0_34_account_multiple() { // Create multiple test accounts with different configurations let accounts = vec![ Account::new("test1", "pass1", Role::SecurityAdministrator, "name1".to_string(), "dept1".to_string(), Some("en".to_string()), // with language Some(vec![IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1))]), // with access restriction Some(2), // with session limit None).unwrap(), Account::new("test2", "pass2", Role::SecurityAdministrator, "name2".to_string(), "dept2".to_string(), None, None, None, None).unwrap(), ]; // Test migration and verify results // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)Cargo.toml(1 hunks)src/account.rs(6 hunks)src/migration.rs(10 hunks)src/tables/accounts.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- src/account.rs
- src/tables/accounts.rs
- CHANGELOG.md
🔇 Additional comments (2)
src/migration.rs (2)
139-143: LGTM! Version upgrade follows established patterns.
The migration entry is correctly added with proper version requirements and follows the established naming conventions.
219-278: LGTM! Account migration implementation is robust and follows best practices.
The migration logic correctly:
- Deserializes existing accounts
- Converts them to the new format with the theme field
- Handles all accounts in the database
- Uses proper error handling
| ### Added | ||
|
|
||
| - Added `Account::theme` field to represent user's selected screen color theme | ||
| on the user interface. | ||
|
|
There was a problem hiding this comment.
This new item should be included in "Unreleased"
There was a problem hiding this comment.
In addition, a new version comparison link should be included in the bottom part.
There was a problem hiding this comment.
I forgot to modify the changelog after the latest main rebase. Sorry. I modified it right away and pushed the new commit.
Cargo.toml
Outdated
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "review-database" | |||
| version = "0.33.0" | |||
| version = "0.33.1-alpha.1" | |||
There was a problem hiding this comment.
Since the modification is backward incompatible, we should bump minor version, 0.34.0-alpha.1
There was a problem hiding this comment.
I replaced the version with 0.34.0-alpha.1 and have pushed the new commit.
div-seungha
force-pushed
the
div/add-dark-mode
branch
from
dbf1b44 to
d697029
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/migration.rs (2)
266-278: Consider optimizing memory usage during migration.The current implementation loads all accounts into memory before writing them back. For large datasets, this could lead to high memory usage.
Consider a streaming approach:
- let mut accounts = vec![]; - for (key, old_value) in raw.iter_forward()? { - let old = bincode::DefaultOptions::new().deserialize::<OldAccount>(&old_value)?; - raw.delete(&key)?; - accounts.push(old.into()); - } - for account in accounts { - map.insert(&account)?; - } + for (key, old_value) in raw.iter_forward()? { + let old = bincode::DefaultOptions::new().deserialize::<OldAccount>(&old_value)?; + let account: Account = old.into(); + map.insert(&account)?; + raw.delete(&key)?; + }
3755-3856: Enhance test coverage for account migration.While the basic migration path is tested, consider adding tests for:
- Error handling (malformed data, serialization failures)
- Migration of multiple accounts
- Accounts with various field combinations
Example additional test cases:
#[test] fn migrate_0_34_account_error_handling() { // Test with malformed data // Test with serialization failures } #[test] fn migrate_0_34_account_multiple() { // Test migrating multiple accounts } #[test] fn migrate_0_34_account_field_variations() { // Test accounts with different language settings // Test accounts with different roles // Test accounts with different access restrictions }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(9 hunks)Cargo.toml(1 hunks)src/account.rs(6 hunks)src/migration.rs(10 hunks)src/tables/accounts.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- CHANGELOG.md
- src/account.rs
- src/tables/accounts.rs
🔇 Additional comments (2)
src/migration.rs (2)
219-221: LGTM: Migration version upgrade function follows the established pattern.
The function correctly delegates the migration to the account-specific migration function.
230-264: LGTM: Account conversion logic correctly handles the new theme field.
The conversion implementations properly:
- Maintain all existing fields
- Initialize the new
themefield asNone
src/migration.rs
Outdated
| fn migrate_0_34_account(store: &super::Store) -> Result<()> { | ||
| use bincode::Options; | ||
| use chrono::{DateTime, Utc}; | ||
|
|
||
| use crate::account::{PasswordHashAlgorithm, Role, SaltedPassword}; | ||
| use crate::types::Account; | ||
|
|
||
| #[derive(Deserialize, Serialize)] | ||
| pub struct OldAccount { | ||
| pub username: String, | ||
| password: SaltedPassword, | ||
| pub role: Role, | ||
| pub name: String, | ||
| pub department: String, | ||
| pub language: Option<String>, | ||
| creation_time: DateTime<Utc>, | ||
| last_signin_time: Option<DateTime<Utc>>, | ||
| pub allow_access_from: Option<Vec<IpAddr>>, | ||
| pub max_parallel_sessions: Option<u32>, | ||
| password_hash_algorithm: PasswordHashAlgorithm, | ||
| password_last_modified_at: DateTime<Utc>, | ||
| } | ||
|
|
||
| impl From<OldAccount> for Account { | ||
| fn from(input: OldAccount) -> Self { | ||
| Self { | ||
| username: input.username, | ||
| password: input.password, | ||
| role: input.role, | ||
| name: input.name, | ||
| department: input.department, | ||
| language: input.language, | ||
| theme: None, | ||
| creation_time: input.creation_time, | ||
| last_signin_time: input.last_signin_time, | ||
| allow_access_from: input.allow_access_from, | ||
| max_parallel_sessions: input.max_parallel_sessions, | ||
| password_hash_algorithm: input.password_hash_algorithm, | ||
| password_last_modified_at: input.password_last_modified_at, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let map = store.account_map(); | ||
| let raw = map.raw(); | ||
| let mut accounts = vec![]; | ||
| for (key, old_value) in raw.iter_forward()? { | ||
| let old = bincode::DefaultOptions::new().deserialize::<OldAccount>(&old_value)?; | ||
| raw.delete(&key)?; | ||
| accounts.push(old.into()); | ||
| } | ||
| for account in accounts { | ||
| map.insert(&account)?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in the account migration function.
While the migration logic is correct, consider enhancing the error handling:
- Replace
into()with explicit error handling to avoid potential panics. - Add specific error types for migration failures.
Consider this improvement:
fn migrate_0_34_account(store: &super::Store) -> Result<()> {
use bincode::Options;
use chrono::{DateTime, Utc};
use crate::account::{PasswordHashAlgorithm, Role, SaltedPassword};
use crate::types::Account;
let map = store.account_map();
let raw = map.raw();
let mut accounts = vec![];
for (key, old_value) in raw.iter_forward()? {
- let old = bincode::DefaultOptions::new().deserialize::<OldAccount>(&old_value)?;
- raw.delete(&key)?;
- accounts.push(old.into());
+ match bincode::DefaultOptions::new().deserialize::<OldAccount>(&old_value) {
+ Ok(old) => {
+ raw.delete(&key)?;
+ accounts.push(old.into());
+ }
+ Err(e) => {
+ return Err(anyhow::anyhow!("Failed to deserialize account: {}", e));
+ }
+ }
}
for account in accounts {
- map.insert(&account)?;
+ map.insert(&account).map_err(|e| {
+ anyhow::anyhow!("Failed to insert migrated account {}: {}", account.username, e)
+ })?;
}
Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn migrate_0_34_account(store: &super::Store) -> Result<()> { | |
| use bincode::Options; | |
| use chrono::{DateTime, Utc}; | |
| use crate::account::{PasswordHashAlgorithm, Role, SaltedPassword}; | |
| use crate::types::Account; | |
| #[derive(Deserialize, Serialize)] | |
| pub struct OldAccount { | |
| pub username: String, | |
| password: SaltedPassword, | |
| pub role: Role, | |
| pub name: String, | |
| pub department: String, | |
| pub language: Option<String>, | |
| creation_time: DateTime<Utc>, | |
| last_signin_time: Option<DateTime<Utc>>, | |
| pub allow_access_from: Option<Vec<IpAddr>>, | |
| pub max_parallel_sessions: Option<u32>, | |
| password_hash_algorithm: PasswordHashAlgorithm, | |
| password_last_modified_at: DateTime<Utc>, | |
| } | |
| impl From<OldAccount> for Account { | |
| fn from(input: OldAccount) -> Self { | |
| Self { | |
| username: input.username, | |
| password: input.password, | |
| role: input.role, | |
| name: input.name, | |
| department: input.department, | |
| language: input.language, | |
| theme: None, | |
| creation_time: input.creation_time, | |
| last_signin_time: input.last_signin_time, | |
| allow_access_from: input.allow_access_from, | |
| max_parallel_sessions: input.max_parallel_sessions, | |
| password_hash_algorithm: input.password_hash_algorithm, | |
| password_last_modified_at: input.password_last_modified_at, | |
| } | |
| } | |
| } | |
| let map = store.account_map(); | |
| let raw = map.raw(); | |
| let mut accounts = vec![]; | |
| for (key, old_value) in raw.iter_forward()? { | |
| let old = bincode::DefaultOptions::new().deserialize::<OldAccount>(&old_value)?; | |
| raw.delete(&key)?; | |
| accounts.push(old.into()); | |
| } | |
| for account in accounts { | |
| map.insert(&account)?; | |
| } | |
| Ok(()) | |
| } | |
| fn migrate_0_34_account(store: &super::Store) -> Result<()> { | |
| use bincode::Options; | |
| use chrono::{DateTime, Utc}; | |
| use crate::account::{PasswordHashAlgorithm, Role, SaltedPassword}; | |
| use crate::types::Account; | |
| #[derive(Deserialize, Serialize)] | |
| pub struct OldAccount { | |
| pub username: String, | |
| password: SaltedPassword, | |
| pub role: Role, | |
| pub name: String, | |
| pub department: String, | |
| pub language: Option<String>, | |
| creation_time: DateTime<Utc>, | |
| last_signin_time: Option<DateTime<Utc>>, | |
| pub allow_access_from: Option<Vec<IpAddr>>, | |
| pub max_parallel_sessions: Option<u32>, | |
| password_hash_algorithm: PasswordHashAlgorithm, | |
| password_last_modified_at: DateTime<Utc>, | |
| } | |
| impl From<OldAccount> for Account { | |
| fn from(input: OldAccount) -> Self { | |
| Self { | |
| username: input.username, | |
| password: input.password, | |
| role: input.role, | |
| name: input.name, | |
| department: input.department, | |
| language: input.language, | |
| theme: None, | |
| creation_time: input.creation_time, | |
| last_signin_time: input.last_signin_time, | |
| allow_access_from: input.allow_access_from, | |
| max_parallel_sessions: input.max_parallel_sessions, | |
| password_hash_algorithm: input.password_hash_algorithm, | |
| password_last_modified_at: input.password_last_modified_at, | |
| } | |
| } | |
| } | |
| let map = store.account_map(); | |
| let raw = map.raw(); | |
| let mut accounts = vec![]; | |
| for (key, old_value) in raw.iter_forward()? { | |
| match bincode::DefaultOptions::new().deserialize::<OldAccount>(&old_value) { | |
| Ok(old) => { | |
| raw.delete(&key)?; | |
| accounts.push(old.into()); | |
| } | |
| Err(e) => { | |
| return Err(anyhow::anyhow!("Failed to deserialize account: {}", e)); | |
| } | |
| } | |
| } | |
| for account in accounts { | |
| map.insert(&account).map_err(|e| { | |
| anyhow::anyhow!("Failed to insert migrated account {}: {}", account.username, e) | |
| })?; | |
| } | |
| Ok(()) | |
| } |
div-seungha
changed the title
theme field in Account to represent user's screen color themetheme field in Account to represent user's screen color theme
div-seungha
force-pushed
the
div/add-dark-mode
branch
from
d697029 to
1b0802d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/migration.rs (2)
3794-3812: Consider adding test coverage for theme field initialization.While the conversion implementation is correct, consider adding a test case that explicitly verifies the theme field is properly initialized to None during migration.
Would you like me to help generate the additional test case?
3767-3868: Enhance test coverage for theme field.The test verifies the basic migration flow but could be enhanced to explicitly assert that the theme field is initialized to None after migration.
Consider adding this assertion:
assert_eq!(account, Some(test)); +// Explicitly verify theme field initialization +assert_eq!(account.unwrap().theme, None);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(9 hunks)Cargo.toml(1 hunks)src/account.rs(6 hunks)src/migration.rs(11 hunks)src/tables/accounts.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- src/account.rs
- src/tables/accounts.rs
- CHANGELOG.md
🔇 Additional comments (3)
src/migration.rs (3)
41-41: LGTM: Version requirement update is appropriate.
The version requirement update follows semantic versioning practices and correctly defines the compatible version range for the database migration.
219-221: LGTM: Clean migration function structure.
The migration function follows the established pattern of version-specific migrations.
223-290: LGTM: Account migration implementation is robust.
The migration logic properly handles:
- Deserialization of old accounts
- Error handling with descriptive messages
- Conversion to new account format with theme field
div-seungha
changed the title
theme field in Account to represent user's screen color themetheme field in Account to represent user's screen color theme
div-seungha
force-pushed
the
div/add-dark-mode
branch
3 times, most recently
from
ad78b98 to
bc41409
Compare
src/migration.rs
Outdated
| @@ -38,7 +38,7 @@ use crate::{Agent, AgentStatus, Giganto, Indexed, IterableMap}; | |||
| /// // the database format won't be changed in the future alpha or beta versions. | |||
| /// const COMPATIBLE_VERSION: &str = ">=0.5.0-alpha.2,<=0.5.0-alpha.4"; | |||
| /// ``` | |||
| const COMPATIBLE_VERSION_REQ: &str = ">=0.30.0,<0.34.0-alpha"; | |||
| const COMPATIBLE_VERSION_REQ: &str = ">0.33.1,<=0.34.0-alpha.1"; | |||
There was a problem hiding this comment.
I think it should be
>=0.34.0-alpha.1,<0.34.0-alpha.2
My reasoning behind is that a potential 0.33.2 release is not compatible with 0.34.0-alpha.1.
There was a problem hiding this comment.
I have made the changes as you suggested.
src/migration.rs
Outdated
| @@ -136,6 +136,11 @@ pub fn migrate_data_dir<P: AsRef<Path>>(data_dir: P, backup_dir: P) -> Result<() | |||
| Version::parse("0.30.0")?, | |||
| migrate_0_29_to_0_30_0, | |||
| ), | |||
| ( | |||
| VersionReq::parse(">=0.33.1,<0.34.0-alpha.1")?, | |||
There was a problem hiding this comment.
I think it should be
VersionReq::parse(">=0.30.0,<0.34.0-alpha.1")?,
There was a problem hiding this comment.
I have modified that line as you suggested.
src/migration.rs
Outdated
| ( | ||
| VersionReq::parse(">=0.33.1,<0.34.0-alpha.1")?, | ||
| Version::parse("0.34.0-alpha.1")?, | ||
| migrate_0_33_to_0_34_0, |
There was a problem hiding this comment.
I think the same would be updated to migrate_0_30_to_0_34_0 accordingly.
There was a problem hiding this comment.
I have modified that line as you suggested.
div-seungha
force-pushed
the
div/add-dark-mode
branch
from
bc41409 to
8f1d371
Compare
src/migration.rs
Outdated
| for (key, old_value) in raw.iter_forward()? { | ||
| match bincode::DefaultOptions::new().deserialize::<OldAccount>(&old_value) { | ||
| Ok(old) => { | ||
| raw.delete(&key)?; | ||
| accounts.push(old.into()); | ||
| } | ||
| Err(e) => { | ||
| return Err(anyhow::anyhow!("Failed to deserialize account: {}", e)); | ||
| } | ||
| } | ||
| } | ||
| for account in accounts { | ||
| map.insert(&account).map_err(|e| { | ||
| anyhow::anyhow!( | ||
| "Failed to insert migrated account {}: {}", | ||
| account.username, | ||
| e | ||
| ) | ||
| })?; | ||
| } |
There was a problem hiding this comment.
What do you think about simplifying this part like https://github.com/petabi/review-database/pull/383/files?diff=split&w=1#diff-df0bb463e010d06b24b197ed640493c03dbd7d37e9f82483eecaeeaed99c2076R1151-R1153 ?
I think it would reduce database operation and simplify the code, without interfering the forward-iteration because the key of the Account is not modified.
There was a problem hiding this comment.
The original code was indeed the same as you suggested. You can check it on this link: #383 (review)
Shortly after creating the PR, I came across the link mentioned above, which raised some points that I found reasonable upon review. After considering coderabbitai's proposal, I made the changes accordingly.
But after faced with your comments, I became more curious about the advantages and disadvantages of both approaches. To gain a clearer understanding, I used ChatGPT to analyze the pros and cons of each logic. Based on this analysis, I concluded that the first approach would be more appropriate for a simple migration function like this one.
As a result, I have reverted the code to its original form as per your suggestion and pushed the updated commit. I really appreciate your valuable insights.
There was a problem hiding this comment.
There was a misunderstanding in the communication of the comments and it was resolved by a separate communication channel. I have force-pushed the new commit. @sophie-cluml
div-seungha
force-pushed
the
div/add-dark-mode
branch
2 times, most recently
from
e72c4ec to
a3ee48b
Compare
|
As I commented here, the CHANGELOG must be automatically formatted by Prettier, an extension of VS Code. I just wanted to point that out. |
div-seungha
force-pushed
the
div/add-dark-mode
branch
from
a3ee48b to
f8c83d3
Compare
I checked the VScode setting again so that the changelog can be re-lined to no more than 80 characters overall and pushedthe modified version commit. |
|
@div-seungha, thanks for the PR! I noticed that you've included some formatting changes for old CHANGELOG entries in this PR. While I appreciate the tidying up, it's generally a good practice to keep unrelated changes separate from the main change being proposed. Would you mind splitting the formatting changes into a separate PR? That way, we can review and merge the main change independently. |
div-seungha
force-pushed
the
div/add-dark-mode
branch
from
f8c83d3 to
19f493c
Compare
I excluded all the modifications that are not related to this PR. I will create a new PR for the changelog formatting. |
Closes: #374
This is a PR that save the users' preferred screen color mode to the
Accountstruct, when users select their preferred color mode on the UI screen. Also, I reflected the changedAccountstruct in the function that updates the account so that review-web could use and update theAccount.(Since review-web is using the update function of review-database to modify contents of accounts)I would appreciate it if you could review it. (cc. @sophie-cluml)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores