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

All: Conflict notes will now populate a new field conflict_parent with the id of the conflict note. #5049

Merged
merged 17 commits into from
Jun 12, 2021
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
6 changes: 5 additions & 1 deletion packages/lib/JoplinDatabase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ export default class JoplinDatabase extends Database {
// must be set in the synchronizer too.

// Note: v16 and v17 don't do anything. They were used to debug an issue.
const existingDatabaseVersions = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38];
const existingDatabaseVersions = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39];

let currentVersionIndex = existingDatabaseVersions.indexOf(fromVersion);

Expand Down Expand Up @@ -888,6 +888,10 @@ export default class JoplinDatabase extends Database {
GROUP BY tags.id`);
}

if (targetVersion == 39) {
queries.push('ALTER TABLE `notes` ADD COLUMN conflict_original_id TEXT NOT NULL DEFAULT ""');
}

const updateVersionQuery = { sql: 'UPDATE version SET version = ?', params: [targetVersion] };

queries.push(updateVersionQuery);
Expand Down
1 change: 1 addition & 0 deletions packages/lib/Synchronizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,7 @@ export default class Synchronizer {
const conflictedNote = Object.assign({}, local);
delete conflictedNote.id;
conflictedNote.is_conflict = 1;
conflictedNote.conflict_original_id = local.id;
await Note.save(conflictedNote, { autoTimestamp: false, changeSource: ItemChange.SOURCE_SYNC });
}
} else if (action == 'resourceConflict') {
Expand Down
53 changes: 53 additions & 0 deletions packages/lib/models/Note.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -332,4 +332,57 @@ describe('models_Note', function() {
expect(sortedNotes3[4].id).toBe(note2.id);
}));

it('should copy conflicted note to target folder and cancel conflict', (async () => {
// Prepare
Copy link
Owner

Choose a reason for hiding this comment

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

Remove

const srcfolder = await Folder.save({ title: 'Source Folder' });
const targetfolder = await Folder.save({ title: 'Target Folder' });

const note1 = await Note.save({ title: 'note', parent_id: srcfolder.id });

const tmpConflictedNote = Object.assign({}, note1);
delete tmpConflictedNote.id;
tmpConflictedNote.is_conflict = 1;
tmpConflictedNote.conflict_original_id = note1.id;
const conflictedNote = await Note.save(tmpConflictedNote, { autoTimestamp: false });
Copy link
Owner

@laurent22 laurent22 Jun 9, 2021

Choose a reason for hiding this comment

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

We don't copy and paste large blocks of code in this codebase, it's just a mess to maintain otherwise. So please do this:

  • Create a function in models/Note called public async createConflictNote(sourceNote:NoteEntity, changeSource:number):Promise<NoteEntity>
  • Make it create the conflict note and save it as in the code above.
  • Then use that function in:
    • Your two test units
    • In lib/Synchronizer.ts

As a general rule if you see you are copying more than two or three lines of code, you should stop and think if the code can be refactored. Such refactoring improves the code quality because it wraps and names (using the function name) business logic and makes it re-usable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be good now.


// COPY File
Copy link
Owner

Choose a reason for hiding this comment

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

Your comment explains nothing and is even confusing since we aren't moving files but notes, so please remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Also no idea myself where did "File" come from 😂 sorry about that

const note2 = await Note.copyToFolder(conflictedNote.id, targetfolder.id);

// Targed should be copied and have conflict removed.
expect(note2.id === conflictedNote.id).toBe(false); // ID must be different.
expect(note2.title).toBe(conflictedNote.title);
expect(note2.is_conflict).toBe(0);
expect(note2.conflict_original_id).toBe('');
expect(note2.parent_id).toBe(targetfolder.id);

// Source conflict should stay as is
expect(conflictedNote.is_conflict).toBe(1);
expect(conflictedNote.conflict_original_id).toBe(note1.id);
expect(conflictedNote.parent_id).toBe(srcfolder.id);
Copy link
Owner

Choose a reason for hiding this comment

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

If you want to test the output of createConflictNote, it should be in a separate test, so please remove this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move it to a separate test.

}));

it('should move conflicted note to target folder and cancel conflict', (async () => {
// Prepare
Copy link
Owner

Choose a reason for hiding this comment

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

Remove

const srcFolder = await Folder.save({ title: 'Source Folder' });
const targetFolder = await Folder.save({ title: 'Target Folder' });
const note1 = await Note.save({ title: 'note', parent_id: srcFolder.id });

const tmpConflictedNote = Object.assign({}, note1);
delete tmpConflictedNote.id;
tmpConflictedNote.is_conflict = 1;
tmpConflictedNote.conflict_original_id = note1.id;
const conflictedNote = await Note.save(tmpConflictedNote, { autoTimestamp: false });

// Move
Copy link
Owner

Choose a reason for hiding this comment

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

In your comments you should explain why, not what. We know that a function called "Note.moveToFolder" is going to move the note to a folder, and a comment for this is just noise. I don't think you need to explain anything here, so please remove the comment.

const movedNote = await Note.moveToFolder(conflictedNote.id, targetFolder.id);

// Note should be moved over.
expect(movedNote.parent_id).toBe(targetFolder.id);

// Note should have conflicts removed.
expect(movedNote.is_conflict).toBe(0);
expect(movedNote.conflict_original_id).toBe('');

}));

});
2 changes: 2 additions & 0 deletions packages/lib/models/Note.ts
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,7 @@ export default class Note extends BaseItem {
changes: {
parent_id: folderId,
is_conflict: 0, // Also reset the conflict flag in case we're moving the note out of the conflict folder
conflict_original_id: '', // Reset parent id as well.
},
});
}
Expand All @@ -537,6 +538,7 @@ export default class Note extends BaseItem {
id: noteId,
parent_id: folderId,
is_conflict: 0,
conflict_original_id: '',
updated_time: time.unixMs(),
};

Expand Down
1 change: 1 addition & 0 deletions packages/lib/services/database/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export interface NoteEntity {
"created_time"?: number
"updated_time"?: number
"is_conflict"?: number
"conflict_original_id"?: string
"latitude"?: number
"longitude"?: number
"altitude"?: number
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,10 @@ describe('Synchronizer.conflicts', function() {
// the conflicted and original note must be the same in every way, to make sure no data has been lost.
const conflictedNote = conflictedNotes[0];
expect(conflictedNote.id == note2conf.id).toBe(false);
expect(conflictedNote.conflict_original_id).toBe(note2conf.id);
for (const n in conflictedNote) {
if (!conflictedNote.hasOwnProperty(n)) continue;
if (n == 'id' || n == 'is_conflict') continue;
if (n == 'id' || n == 'is_conflict' || n == 'conflict_original_id') continue;
expect(conflictedNote[n]).toBe(note2conf[n]);
}

Expand Down