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

6838 add prefs repo layer #6885

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Conversation

noel-yeldos
Copy link
Contributor

@noel-yeldos noel-yeldos commented Mar 10, 2025

Fixes #6838

πŸ‘©πŸ»β€πŸ’» What does this PR do?

Add preference to changelogtable and Add repo layer for preference table

πŸ’Œ Any notes for the reviewer?

πŸ§ͺ Testing

  • Not sure how to test this

πŸ“ƒ Documentation

  • Part of an epic: documentation will be completed for the feature as a whole
  • No documentation required: no user facing changes or a bug fix which isn't a change in behaviour
  • These areas should be updated or checked:
    1.
    2.

πŸ“ƒ Reviewer Checklist

The PR Reviewer(s) should fill out this section before approving the PR

Breaking Changes

  • No Breaking Changes in the Graphql API
  • Technically some Breaking Changes but not expected to impact any integrations

Issue Review

  • All requirements in original issue have been covered
  • A follow up issue(s) have been created to cover additional requirements

Tests Pass

  • Postgres
  • SQLite
  • Frontend

@Chris-Petty Chris-Petty self-assigned this Mar 10, 2025
#[derive(PartialEq, Debug, Clone, Default)]
pub struct Preference {
pub preference_row: PreferenceRow,
pub store_row: StoreRow,
Copy link
Contributor

Choose a reason for hiding this comment

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

You're not useing this store row for anything, and it's not clear to me that you will need it.
I think probably safer to just remove it for now, and only add it in when it's required for something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here: 5bccdaa

Copy link
Contributor

@Chris-Petty Chris-Petty left a comment

Choose a reason for hiding this comment

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

Noice! I'd do the little refactor that you mentioned that LachΓ© mentioned and fn delete should update the changelog

}
}

pub type PreferenceJoin = (PreferenceRow, StoreRow);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to remove StoreRow here as well?

Comment on lines +76 to +97
let result = final_query
.load::<PreferenceJoin>(self.connection.lock().connection())?
.into_iter()
.map(|(preference_row, _store_row)| Preference { preference_row })
.collect();

Ok(result)
}
}

type BoxedPreferenceQuery = IntoBoxed<'static, InnerJoin<preference::table, store::table>, DBType>;

fn create_filtered_query(filter: Option<PreferenceFilter>) -> BoxedPreferenceQuery {
let mut query = preference::table.inner_join(store::table).into_boxed();

if let Some(f) = filter {
let PreferenceFilter { id } = f;

apply_equal_filter!(query, id, preference::id);
}
query
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to remove StoreRow here as well? I thought we needed storeRow to access store_id?

@noel-yeldos noel-yeldos requested a review from Chris-Petty March 11, 2025 04:19
Copy link
Contributor

@Chris-Petty Chris-Petty left a comment

Choose a reason for hiding this comment

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

luks gud

}

pub fn delete(&self, preference_id: &str) -> Result<Option<i64>, RepositoryError> {
let old_row = self.find_one_by_id(preference_id)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey look it came in use! I knew it'd have something to do with sync πŸ˜†

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prefs Repo Layer
3 participants