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

Fix: Dont overwrite equal drafts #6212

Merged
merged 11 commits into from
Nov 17, 2024
Merged

Fix: Dont overwrite equal drafts #6212

merged 11 commits into from
Nov 17, 2024

Conversation

Septias
Copy link
Collaborator

@Septias Septias commented Nov 15, 2024

This PR prevents overwriting drafts when the text and file are the same.

close #6211

@Septias Septias requested a review from link2xt November 15, 2024 10:54
Copy link
Collaborator

@link2xt link2xt left a comment

Choose a reason for hiding this comment

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

What if the text is the same, but quote is different?

src/chat.rs Outdated
let self_chat = alice.get_self_chat().await.id;
self_chat.set_draft(&alice, Some(&mut msg)).await.unwrap();
let draft1 = self_chat.get_draft(&alice).await?.unwrap();
tokio::time::sleep(Duration::from_millis(800)).await;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use SystemTime::shift?

@Septias
Copy link
Collaborator Author

Septias commented Nov 15, 2024

Ah true, quote should probably also be checked. We definitely need to update but maybe we can just update the quote and don't touch timestamp.

@Septias Septias force-pushed the sk/dont_overwrite_eq_draft branch from 48cc5a3 to 9bcd84b Compare November 15, 2024 11:57
@link2xt
Copy link
Collaborator

link2xt commented Nov 15, 2024

Ah true, quote should probably also be checked. We definitely need to update but maybe we can just update the quote and don't touch timestamp.

Why not change the timestamp if the quote is updated?

Instead of all these checks that need to be synchronized, can UPDATE condition after WHERE be modified to check if the values that are updated are all the same?

@iequidoo
Copy link
Collaborator

Another option is to make it a transaction which first updates everything except the timestamp and if the number of changed rows != 0, updates the timestamp as well.

src/chat.rs Outdated
),
)?;
if affected_rows > 0 {
transaction.execute("UPDATE msgs SET timestamp=? WHERE id=?;", (time(),msg.id))?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could update the timestamp in the same UPDATE above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but then we would anyways update the timestamp and hence move that chat to the top, exactly what we are trying to avoid?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not if there is a WHERE check.

@Septias Septias requested a review from link2xt November 16, 2024 14:36
src/chat.rs Outdated
)
.await?;
return Ok(true);
.transaction(|transaction| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no need for transaction anymore.

src/chat.rs Outdated
msg.id,
),
).await?;
if affected_rows > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

return Ok(affected_rows > 0)

@Septias Septias enabled auto-merge (squash) November 17, 2024 08:30
@Septias Septias merged commit 399716a into main Nov 17, 2024
38 checks passed
@Septias Septias deleted the sk/dont_overwrite_eq_draft branch November 17, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not update draft timestamp when the draft is not changed
4 participants