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

Validate XLS rows count before exporting. #612

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

hsmett
Copy link
Member

@hsmett hsmett commented Jul 28, 2023

Prevent an error when reaching 65535 rows during an .XLS file export, which is a hard limit defined by the .XLS format specs.
Display a clear error message prior to the export, returning an http 409 error code.

@hsmett hsmett force-pushed the hsmett-XLS-max-rows branch 4 times, most recently from e404fcb to f318d0b Compare July 28, 2023 12:16
@hsmett hsmett requested a review from genglert July 28, 2023 12:16
@hsmett hsmett force-pushed the hsmett-XLS-max-rows branch from f318d0b to 64da1ed Compare July 28, 2023 12:19
Copy link
Contributor

@genglert genglert left a comment

Choose a reason for hiding this comment

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

Target the branch "v2.5-rc5" please.

CHANGELOG.txt Outdated
Comment on lines 12 to 13
# Prevent an error when reaching 65535 rows during an .XLS file export, which is a hard limit defined by the .XLS
format specs. Display a clear error message prior to the export.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the changelog, I try to write sentences with more literal style, something like "The export for .XLS file now displays an error when...".

Comment on lines 75 to 76
def validate(self, **kwargs):
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe should we add a doc-string with classical "keys" (how to know if we should use "count", "total", "total_count" or "total_number" ?
Or add keywords only arguments with default values like def validate(self, *, total_count=None): (which would be IDE friendly).
I prefer the second solution.

Comment on lines 49 to 51
from ...backends import export_backend_registry
from ...backends.base import ExportBackend
from ...core.exceptions import ConflictError
Copy link
Contributor

Choose a reason for hiding this comment

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

I try to avoid ... in the code base (I use only . & ..).

Comment on lines 549 to 553
self.assertIsNone(export_backend_registry.get_backend_class(TestExportBackend.id))
export_backend_registry._backend_classes[TestExportBackend.id] = TestExportBackend
response = self.assertGET409(self._build_contact_dl_url(doc_type=TestExportBackend.id))
self.assertContains(response, 'TestExportBackend.validate fail', status_code=409)
export_backend_registry._backend_classes = None
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be more straightforward to copy the original _backend_classes value & restore the value in a finally block.
A better solution would be to store the registry in an attribute of the class view, to set our own registry in setupClass() & restore the original one in tearDownClass().

@hsmett hsmett force-pushed the hsmett-XLS-max-rows branch from 64da1ed to 7213414 Compare July 28, 2023 13:36
@hsmett hsmett changed the base branch from main to v2.5-rc5 July 28, 2023 13:38
@hsmett hsmett requested a review from genglert July 28, 2023 13:38
@hsmett hsmett force-pushed the hsmett-XLS-max-rows branch from 7213414 to 1956d32 Compare July 28, 2023 13:42
Base automatically changed from v2.5-rc5 to main August 4, 2023 14:06
@hsmett hsmett force-pushed the hsmett-XLS-max-rows branch from 1956d32 to b2a5418 Compare August 23, 2023 10:12
@hsmett hsmett force-pushed the hsmett-XLS-max-rows branch from b2a5418 to 91d82cf Compare September 8, 2023 09:19
@hsmett hsmett changed the base branch from main to v2.5-rc7 September 8, 2023 09:19
Base automatically changed from v2.5-rc7 to main September 11, 2023 07:57
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.

3 participants