Prevent outgoing scrubbed credit card records

[lougeniaC64]

Fixes two issues:

• Prevents the panic that was occurring in the common.rs/get_outgoing_records
• Updates the outgoing credit card logic to exclude outgoing scrubbed credit card records

Pull Request checklist

• Breaking changes: This PR follows our breaking change policy
  • This PR follows the breaking change policy:
    • This PR has no breaking API changes, or
    • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
• Quality: This PR builds and tests run cleanly
  • Note:
    • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
    • If this pull request includes a breaking change, consider cutting a new release after merging.
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
  • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
• Dependencies: This PR follows our dependency management guidelines
  • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

[lougeniaC64]

This was my original change. As expected it still failed because the issue is that the local record doesn't have an associated mirror record.

[lougeniaC64]

This change prevents the panic.

[mhammond]

not sure this is the right error though - can't we use the original error?

[lougeniaC64]

It's not. I think I need to implement rusqlite::Error: From<error::Error> to call e.into() here. One of the reasons I slapped a WIP on this.

[mhammond]

It looks like that should work today - https://github.com/mozilla/application-services/blob/main/components/autofill/src/error.rs#L39-L40

[lougeniaC64]

It doesn't for me.

[mhammond]

The problem here is that query_map() want to return a rusqlite error, not one of ours. So the message is telling you we don't have the reverse - ie, we can't work out how to go from our error to a rusqlite error, which is expected.

Rust is complicated. It took me a long time to work this out, but I think you want something like:

fn get_outgoing_records(
    conn: &Connection,
    sql: &str,
    record_from_data_row: &dyn Fn(&Row<'_>) -> Result<(OutgoingBso, i64)>,
) -> anyhow::Result<Vec<(OutgoingBso, i64)>> {
    conn.prepare(sql)?
        .query_map([], |row| Ok(record_from_data_row(row)))?
        // So now we have Ok(Result<T, E: rusqlite::Error>) - unwrap the OK, then map the rusqlite error to ours.
        .map(|r| {
            r.unwrap().map_err(|e| {
                log::error!(
                    "Failed to retrieve a record from a row with the following error: {}",
                    e
                );
                e.into()
            })
        })
        .collect::<std::result::Result<Vec<_>, _>>()
}

although I suspect there's a better way,

[lougeniaC64]

Thanks! My rust is definitely rusty atm and I'm not sure if a better solution would have come to me when it wasn't. Either way I'll refactor this a bit and see if I can come up with a better solution.

[mhammond]

another possibility would be for scrub_encrypted_data to set the sync_change_counter to zero, and a mini-migration to set all cards with empty numbers to have a change counter of zero too. What you have above seems fine though.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.08%. Comparing base (592063c) to head (0611eea).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6143   +/-   ##
=======================================
  Coverage   84.08%   84.08%           
=======================================
  Files         117      117           
  Lines       15629    15629           
=======================================
  Hits        13141    13141           
  Misses       2488     2488           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mhammond]

Thanks!