-
Notifications
You must be signed in to change notification settings - Fork 47
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
[IMP] model: create snapshot when loading xlsx file #5320
base: 18.0
Are you sure you want to change the base?
Conversation
src/model.ts
Outdated
@@ -281,7 +281,7 @@ export class Model extends EventBus<any> implements CommandDispatcher { | |||
|
|||
this.joinSession(); | |||
|
|||
if (config.snapshotRequested) { | |||
if (config.snapshotRequested || data["[Content_Types].xml"]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should not snapshot if the spreadsheet is in read-only mode (because the user most cannot update the spreadsheet in any way)
d4ac9cd
to
1bbd91a
Compare
tests/model/model.test.ts
Outdated
@@ -366,4 +369,22 @@ describe("Model", () => { | |||
], | |||
}); | |||
}); | |||
|
|||
test("snapshot when importing xlsx file", async () => { | |||
jest.spyOn(console, "warn").mockImplementation(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a global handler to make the tests fail in case of console.error or console.warn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - exactly :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this global handler, but if I comment this line, the test does not fail. Expected ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird, it was failing before 🤷
if (config.snapshotRequested) { | ||
if ( | ||
config.snapshotRequested || | ||
(data["[Content_Types].xml"] && this.config.mode !== "readonly") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test for the readonly flag
1bbd91a
to
4804a36
Compare
Before this commit, when uploading an XLSX file, the code in the Odoo repository would create a spreadsheet document with data that is encoded in xml. Whenever the file is loaded, we parse the stringified xml to convert it to the o-spreadsheet json structure. The problem is that this conversion can be quite slow and costly when the XLSX file is large. This commit mitigates the issue by saving a snapshot once the XLSX file is loaded for the first time. Task: 4080514
4804a36
to
adb4f86
Compare
Before this commit, when uploading an XLSX file, the code in the Odoo repository would create a spreadsheet document with data that is encoded in xml. Whenever the file is loaded, we parse the stringified xml to convert it to the o-spreadsheet json structure. The problem is that this conversion can be quite slow and costly when the XLSX file is large.
This commit mitigates the issue by saving a snapshot once the XLSX file is loaded for the first time.
Task: 4080514
review checklist