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

Refactor CSV export #4293

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

Refactor CSV export #4293

wants to merge 20 commits into from

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Oct 14, 2024

This adds last_active and status columns to the team CSV export. It also includes a refactoring of the export handler. I manually tested performance of the new handler, and for 500 users it's very similar to before despite the extra query. For larger teams it should be comparatively faster.

We have had a discussion on whether or not to make a new version of this endpoint. Adding columns at the end of the previous CSV format is quite unlikely to break scripts, so in the end I decided not to make a new version and keep things simple. In the future, we might consider further improving the export functionality by making it asynchronous for large teams, and possibly making columns user-selectable.

https://wearezeta.atlassian.net/browse/WPB-11368

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@pcapriotti pcapriotti force-pushed the team-info-stern branch 2 times, most recently from 19c4ac0 to 81fcc17 Compare October 16, 2024 07:49
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 16, 2024
@pcapriotti pcapriotti marked this pull request as ready for review October 17, 2024 07:30
@mdimjasevic mdimjasevic self-requested a review October 17, 2024 12:19
@@ -374,6 +374,7 @@ lookupName u =
fmap runIdentity
<$> retry x1 (query1 nameSelect (params LocalQuorum (Identity u)))

-- TODO: remove this
Copy link
Contributor

Choose a reason for hiding this comment

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

Something still to be done?

case mFromInvitation of
Just ts -> pure $ Just ts
Nothing -> do
-- TODO: make this a single user query
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still to be done or you've changed your mind?

Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Please address or drop the two TODOs. Otherwise this looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants