politics-rewired / Spoke

Politics Rewired's fork of Spoke
GNU General Public License v3.0
34 stars 16 forks source link

feat(exports)!: export multiple campaigns #1641

Open henryk1229 opened 12 months ago

henryk1229 commented 12 months ago

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 https://github.com/politics-rewired/Spoke/assets/47220718/0c148971-4c9c-4b47-b0a7-562bb3a07136 https://github.com/politics-rewired/Spoke/assets/47220718/95afac41-8bd0-4e61-81cc-81aee9a657b0

Documentation Changes

Documentation change TK

Checklist:

henryk1229 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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?

kennyycheng commented 11 months ago

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:

Campaign Status bubbles image

Campaign Info properties

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!

hiemanshu commented 11 months ago

@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 commented 11 months ago
  • 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?

ajohn25 commented 11 months ago

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 👍🏾