campaigns: Allow filtering/sorting of campaigns and changesets in campaigns

[mrnugget]

This is a feature request from a customer:

• Sort changesets on a campaign page:
  • status: open/closed/merged
  • CI status
  • review status
  • ?
• Filter changesets on a campaign page:
  • status: open/closed/merged
  • CI status
  • review status
  • ?
• Filter Campaigns on the campaigns overview by:
  • status: open/closed
  • ?
• Sort Campaigns on the campaigns overview by:
  • status: open/closed
  • creation date
  • last updated

For all of these we probably need:

• GraphQL schema
• pagination of changesets
• pagination of campaigns

Regarding the backend implementation: we need to think about whether this means
the end of our "denormalization" (we currently keep everything in
changesets.metadata) and whether we need to normalize the data we want to
filter by to separate columns.

[tsenart]

Please estimate this from a backend perspective. If it's easier to split this issue into two, do so.

[ryanslade]

Review and CI state are both computed in code so if we want to perform the queries in SQL we'd need to denormalise. I'm not sure we'd be able to do it via a migration unfortunately, unless we run some custom code on startup? Is that something we've done before? We could just default everything to an "unknown" state initially and have it populate as we sync.

Having the columns will also allow us to easily display a nice summary line at the top showing the total number in each state.

I'm going conservatively estimate this at 3 days for the backend work

[tsenart]

Review and CI state are both computed in code so if we want to perform the queries in SQL we'd need to denormalise.

Dunno what @mrnugget's take on this is, but if we need to do this filtering performantly, would we need to filter already in the database layer? If so, it might be good to normalize this data.

Otherwise ...

unless we run some custom code on startup? Is that something we've done before?

Yes, we have done this before. It's tricky, but doable.

[ryanslade]

To be clear, I am suggesting that we create separate columns. We would continue to store the metadata but then on each sync or webhook we'd need to recompute the columns in code and update them. So we'd be denormalising further by introducing new columns.

[tsenart]

👍

[mrnugget]

In the issue here I wrote

Regarding the backend implementation: we need to think about whether this means
the end of our "denormalization" (we currently keep everything in
changesets.metadata) and whether we need to normalize the data we want to
filter by to separate columns.

and now that @ryanslade has arrived at the same thought I think it's time to do this:

1. Create separate columns (external_review_state)
2. Populate those columns on sync (just like we create changeset_events rows from (&Changset).Events(), we can populate changesets.external_review_state from (&Changeset).ReviewState()

We could just default everything to an "unknown" state initially and have it populate as we sync.

That seems fine to me, since

1. to display the attribute we can still fall back to the metadata column
2. they'd be populated on the next sync

But regarding this point:

I'm not sure we'd be able to do it via a migration unfortunately, unless we run some custom code on startup?

Take a look at @unknwon's migration here: https://github.com/sourcegraph/sourcegraph/pull/8495/files#diff-2368a32b38eca2bb2b142233c2c87966

Does that help with answering whether it's possible or not? (Asking because I might be missing some thing here that you already thought of)

[tsenart]

Populate those columns on sync

I think it'd be easier to reason and work with this problem if we did this at migration time. Check how @unknwon is doing this for the new private field in the repo table coming from the JSONB metadata:

sourcegraph/migrations/1528395653_repo_normalize_visibility_metadata.up.sql

Line 12 in 9c0501d

FOR t_row IN t_cursor LOOP

[tsenart]

Forget my last comment, didn't read your response fully @mrnugget.

[ryanslade]

During our last sync it was decided that filtering is enough for now and we can add sorting later if it's requested.

Filtering is merged so closing this issue.

[mrnugget]

Please create a separate issue for the sorting and add it to the next milestone then so we don't lose it. We can always take it out when planning.

[ryanslade]

When we chatted with @christinaforney the other day it sounded like it wasn't planned until someone asks for it

[christinaforney]

I will validate if the filtering is sufficient with some users and will directly ask if this solves their needs or if sorting is necessary. No need to file for now, if it's important enough we won't forget :)