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

Add AfterChunk event for chunked exports #4037

Merged
merged 8 commits into from Jan 16, 2024

Conversation

sebestenyb
Copy link
Contributor

@sebestenyb sebestenyb commented Nov 27, 2023

fixes #3606

Add AfterChunk event for chunked exports

This PR allows the AfterChunk event to fire on chunked queued exports.

#3634

1️⃣ Why should it be added? What are the benefits of this change?

With this, it is possible to

  • showing the progress to the user
  • estimate time for an export

2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.

No

3️⃣ Does it include tests, if possible?

No

4️⃣ Any drawbacks? Possible breaking changes?

No breaking changes.

5️⃣ Mark the following tasks as done:

  • Checked the codebase to ensure that your feature doesn't already exist.
  • Take note of the contributing guidelines.
  • Checked the pull requests to ensure that another person hasn't already submitted a fix.
  • Added tests to ensure against regression.

6️⃣ Thanks for contributing! 🙌

@patrickbrouwers
Copy link
Member

@sebestenyb
Copy link
Contributor Author

Could you add a test for this event in https://github.com/SpartnerNL/Laravel-Excel/blob/3.1/tests/Concerns/WithEventsTest.php

Hi, thanks for considering this PR. I have added a test, I hope this is how you wanted.
It was a bit tricky because this event was only raised from a queued context, so couldn't use closure based event handlers.

Cheers, Sebi

@sebestenyb
Copy link
Contributor Author

Hi @patrickbrouwers, do you need anything else here?

public function export_chunked_events_get_called()
{
$this->loadLaravelMigrations(['--database' => 'testing']);
User::query()->create([
Copy link
Member

Choose a reason for hiding this comment

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

If we could create 2 users here,
Do a chunk export with chunk size 1

Then we could do an additional assert that the chunk event was raised twice, I think. Could you check if that's possible.

@patrickbrouwers patrickbrouwers merged commit 104df8a into SpartnerNL:3.1 Jan 16, 2024
16 checks passed
@patrickbrouwers
Copy link
Member

Improved the test myself

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.

[Bug]: Listeners are empty in writer within AppendQueryToSheet when chunking exports
2 participants