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

feat(exports)!: export multiple campaigns #1641

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

Conversation

henryk1229
Copy link
Contributor

@henryk1229 henryk1229 commented Jul 20, 2023

Description

This PR makes it possible for admins to export multiple campaigns at once. It also makes it possible for admins to filter campaigns by title, and introduces some design-related changes to the CampaignList view, to make it easier to incorporate the new functionality into the AdminCampaignList component without overwhelming users.

Motivation and Context

It closes #915

How Has This Been Tested?

Behaviorally

Screenshots (if appropriate):

Screenshot 2023-08-04 at 2 47 56 PMScreenshot 2023-08-04 at 2 48 19 PMScreenshot 2023-08-04 at 2 47 47 PM
Screen.Recording.2023-08-04.at.2.54.09.PM.mov
Untitled.mov

Documentation Changes

Documentation change TK

Checklist:

  • My change requires a change to the documentation.
  • I have included updates for the documentation accordingly.

@hiemanshu hiemanshu added this to the 8.0.0 milestone Jul 26, 2023
@hiemanshu hiemanshu changed the title feat(exports): export multiple campaigns feat!(exports): export multiple campaigns Jul 26, 2023
@hiemanshu hiemanshu changed the title feat!(exports): export multiple campaigns feat(exports)!: export multiple campaigns Jul 26, 2023
@henryk1229 henryk1229 marked this pull request as ready for review July 27, 2023 01:27
@henryk1229
Copy link
Contributor Author

A possible follow-up issue: sending one email per bulk export operation. This was requested by @kohlee when speccing out the task originally, but (to the best of my knowledge) the way we currently run tasks via the job_request table and graphile-worker would make it very hard to implement this in an effective way.

I spent a half-day trying to make it work with the current configuration, but I think enabling this functionality might require a larger refactor to our job-handling architecture. Happy to spin out an issue and do some more speccing there, if folks think it's worth it - @bchrobot @hiemanshu thoughts?

@henryk1229
Copy link
Contributor Author

One other note - initially @kohlee requested being able to export multiple van campaigns, but @hiemanshu thought it might be too much work. I think we could handle multiple van exports using a similar approach as we use here for spoke exports, unless I'm missing something. @hiemanshu I feel like it might be another one or two hours work, unless I'm missing something?

@bchrobot
Copy link
Member

bchrobot commented Jul 27, 2023

A follow up issue sounds right to me so as to unblock this. But I defer to @kohlee on how much of a usability issue multiple emails is.

My thoughts on implementation are not that different from what you're currently doing with the new addExportMultipleCampaigns task. Instead of having the task complete after queuing the other jobs, it would update the payload with the IDs for the exportCampaign tasks it created. Then the job becomes polling the completion status of those child jobs (or the child jobs could update the status of the parent job when they each complete, using SELECT FOR UPDATE to avoid race conditions).

Re: VAN campaigns, maybe punt that to a follow up issue (again just to unblock this being merged and released) with some detail about what minimal change set is needed to also support VAN exports?

Copy link
Contributor

@ajohn25 ajohn25 left a comment

Choose a reason for hiding this comment

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

1 larger question about job tables at the end, and an assortment of smaller things, but nicee. Excited for the new layout 🥳

libs/gql-schema/schema.ts Outdated Show resolved Hide resolved
src/components/ExportMultipleCampaignDataDialog.tsx Outdated Show resolved Hide resolved
src/containers/AdminCampaignList.jsx Show resolved Hide resolved
src/containers/CampaignList/components/CampaignListRow.tsx Outdated Show resolved Hide resolved
src/containers/CampaignList/utils.ts Show resolved Hide resolved
src/server/tasks/export-campaign.ts Outdated Show resolved Hide resolved
src/server/tasks/export-campaign.ts Outdated Show resolved Hide resolved
src/server/tasks/export-multiple-campaigns.ts Show resolved Hide resolved
@kennyycheng
Copy link

I like a lot of Aashish’s suggestions above, especially the started/not started, reducing title size, and removing due date!

I had some smaller design adjustments and nits to add as well if possible:

  • Is it possible to change the font to Karla? It might be a larger issue, since a lot of the MUI components (at least I think that’s what it is?) look like they’re not taking Karla and are still using Roboto/Sans-serif? If it's an easy switch that'd be awesome, but otherwise maybe we can investigate further outside of this?

Campaign Status bubbles
image

  • Add a light Background color (green: #DFF0DF , orange: #FFF2E9) and radius
  • Change checked icon to filled
  • Change “Alert” icon to “Incomplete” icon and change color from yellow to orange (#FF781D) for better contrast
    • Unless folks have stronger feelings about this. In general, I think I’d want to avoid desensitizing folks to the alert icon/color if we don’t have to. Seems like it's important to call attention without being like “SOS emergency!”
      progress_activity_FILL1_wght600_GRAD0_opsz48

Campaign Info properties

  • Add tooltip when hovering the icons “Created by” “Teams” “Campaigns”
  • Make all of the icons and text a lighter gray (#666666?) just to deprioritize compared to other info
  • Change campaign groups icon to campaigns "megaphone" (Unless anyone has other suggestions) - want to differentiate from Description icon below
    campaign_FILL0_wght400_GRAD0_opsz48
  • Instead of "Description:" label, replace with description icon for consistency with other fields. Add tooltip that says description on hover as well.
    description_FILL0_wght400_GRAD0_opsz48
  • Adjust left padding on the “Created by” icon so it aligns with the campaign title more neatly
  • Capitalize “ID:” and use a lighter gray for the label and number (something like #666666 maybe?)
  • In the export dialog, can we make the campaign names smaller? They can just be body/paragraph size. + if we're able to do the same treatment as ^ where "ID: ####" is lighter gray and capitalized.

A lot of these are pretty small details, and items can be reprioritized if anything ends up blowing up the scope, but just wanted to make a few edits after peeking at this a bit more. In any case though, looks awesome, and I'm excited for the changes!

libs/gql-schema/schema.ts Outdated Show resolved Hide resolved
libs/gql-schema/schema.ts Outdated Show resolved Hide resolved
libs/gql-schema/schema.ts Outdated Show resolved Hide resolved
src/containers/AdminCampaignList.jsx Outdated Show resolved Hide resolved
interface CampaignDetailsProps {
id: string;
description: string;
creatorName: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

The field is nullable. So we should work with that assumption. Maybe inserting campaigns through API or something?

@hiemanshu
Copy link
Contributor

@henryk1229 Overall looks good! A few nit picks / suggestions.

I would create a new PR on top of this, to unblock this from being merged, to send all exports in 1 email.

@henryk1229
Copy link
Contributor Author

  • Is it possible to change the font to Karla? It might be a larger issue, since a lot of the MUI components (at least I think that’s what it is?) look like they’re not taking Karla and are still using Roboto/Sans-serif? If it's an easy switch that'd be awesome, but otherwise maybe we can investigate further outside of this?

@kennyycheng @ajohn25 this requires a one-line change tomui-theme.ts, which is easy enough - but I'm wondering if it would be better to separate the change into its own pr, since it would affect every component that uses the material-ui Typography component. I clicked through the routes in the admin sidebar and didn't notice it interfering with any existing css or creating any ui issues, but it might be worth more investigation. @ajohn25 thoughts?

@henryk1229 henryk1229 requested a review from ajohn25 August 4, 2023 19:14
@ajohn25
Copy link
Contributor

ajohn25 commented Aug 4, 2023

I'm wondering if it would be better to separate the change into its own pr, since it would affect every component that uses the material-ui Typography component.

this sounds good to me just in case yeah 👍🏾

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.

Support exporting multiple campaigns at once
5 participants