Make Message Review filters shareable by url

[kohlee]

Is your feature request related to a problem? Please describe. Admins are unable to share Message Review urls with fellow users and have the filters pre-populate with values that were present when the url was copied.

Describe the solution you'd like Message Review views should be shareable by url.

Describe alternatives you've considered N/A

Additional context This is supported in MoveOn's fork.

[hiemanshu]

@bchrobot For this I was thinking of moving most of the state into a search params of the URL. So the component reads the values from the URL, and any changes in filters cause a change in the URL, this way the URL can just be copy pasted and shared with anyone else.

I've tried to experiment with a few different ideas, and instead of maintaining multiple sources of truth, moving the source to the URL made the most sense.

[bchrobot]

@hiemanshu I 100% agree with this. I have a few ideas about how this could pair with a larger refactor of message review. I will open a PR with a design doc later today.

[hiemanshu]

@bchrobot following up on this, do you have any other ideas on the refactoring?

[bchrobot]

Current State

Message Review currently looks like:

<AdminIncomingMessageList>
  {/* Workaround for fetching data */}
  <PaginatedUsersRetriever />
  <PaginatedUsersRetriever />
  <PaginatedCampaignsRetriever />

  <IncomingMessageFilter>
    <Card>
      <Toggle />
      <Toggle />
      <SelectField />
      <Autocomplete />
      <Autocomplete />
      <TextField />
      <SelectField />
    </Card>
  </IncomingMessageFilter>

  <IncomingMessageActions />

  <IncomingMessageList>
    <DataTable />
    <ConversationPreviewModal />
  </IncomingMessageList>

  <ReassignmentResultDialog />
</AdminIncomingMessageList>

Drawbacks:

• The top level <AdminIncomingMessageList> handles lots of state, business logic, and layout
• The data retrievers are a clunky work around for an outdated mode of using Apollo Client
• <IncomingMessageFilter> manages the container/layout and business logic for all filter form input elements

Proposal

• Extract business logic and state management into a standalone hook using useReducer()
  • Campaign filter state stored in URL query params (e.g. ?campaign=6&texter=unassigned)
  • Conversation detail modal ID stored in URL query param (e.g. ?activeConversation=5)
• Break up filter component. Container handles layout, individual components handle filter-specific logic (e.g. resolving multiselect dropdown value into a valid GraphQL query input object)
  • Where appropriate, Autocompletes and SelectFields can fetch their data sources (either fetching all results as we do now or pushing search down to server via query input)
• <IncomingMessageActions> can fetch data sources for its Autocomplete components as well
• Pull conversation preview modal up a level
• Update incoming message list to a React v16 compatible data table implementation. The one muiv4 provides is likely sufficient

<AdminIncomingMessageList>
  <IncomingMessageFilter>
    <Grid>
      <MessageStatusSelect />
      <CampaignSelect />
      <TexterSelect />
      <TagSelect />
    </Grid>
  </IncomingMessageFilter>

  <IncomingMessageActions />

  <IncomingMessageList>
    <DataTable muiVersion="v4" />
  </IncomingMessageList>

  <ConversationPreviewModal />
  <ReassignmentResultDialog />
</AdminIncomingMessageList>

Questions I still have about my proposal are:

• Is pushing data source fetching down closer to the Autocomplete and SelectFields that consume them a bad idea?
• Should reassignment actions be handled in the top level business logic hook? Or pushed down to <IncomingMessageActions>?
  • Pros of top level: easier to manage/co-locate refreshing state based on operation result (not something that currently happens)
  • Cons of top level: more stuff to shove into the reducer when it could just focus on managing filter state and resulting conversations
  • Pros of pushing it down: it consolidates reassignment into the reassignment component
  • Cons of pushing it down: updating conversations based on operation result requires using Apollo's refetchQueries: [GetConversationsDocument] which feels like tight coupling? refetchQueries could also be a <IncomingMessageActions> prop so that <AdminIncomingMessageList> is the one in control of which queries (e.g. GetConversations) are refetched.
• If reassignment actions are pushed down, then the operation result dialog can also be pushed down

[hiemanshu]

Extract business logic and state management into a standalone hook useReducer

I don't think we want to use a react hook here, useReducer still helps with managing state, and we don't want to manage state for the most part. We'll have a couple of items that use state, but most everything else will be read from the URLSearchParams. For the little state we want to manage, we should be able to just make do with useState

Break up filter component. Container handles layout, individual components handle filter-specific logic (e.g. resolving multiselect dropdown value into a valid GraphQL query input object)

Sounds good! Splitting into similar types grouped together.

can fetch data sources for its Autocomplete components as well Pull conversation preview modal up a level

Sounds good!

Update incoming message list to a React v16 compatible data table implementation. The one muiv4 provides is likely sufficient

Exploring this, the one in v4 is in the lab, and not in core. DataGrid also has a paid pro version.

Is pushing data source fetching down closer to the Autocomplete and SelectFields that consume them a bad idea?

Not at all, the component that needs the data should be able to call the API to fetch it, making the component more atomic.

Should reassignment actions be handled in the top level business logic hook? Or pushed down to ?

This is an interesting question. Looking at the data flow here, IncomingMessageList tells the parent what messages have been selected, and IncomingMessageActions will need to read the parents state to find what messages it should reassign. I'd say for the simpler data flow (this refactor is going to be large as is), we should let the parent manage it for now. We can come back and revisit this idea at a later time.

[bchrobot]

Extract business logic and state management into a standalone hook useReducer

useReducer still helps with managing state, and we don't want to manage state for the most part. We'll have a couple of items that use state, but most everything else will be read from the URLSearchParams.

Sorry, I should have clarified that I was including URL query params in my bucket of "state" - just swapping useState for useQueryParam. Agree that we shouldn't be adding more React state via useState though.

Update incoming message list to a React v16 compatible data table implementation. The one muiv4 provides is likely sufficient

Exploring this, the one in v4 is in the lab, and not in core. DataGrid also has a paid pro version.

Got it -- looks like DataGrid is the path forward in mui v5 as well. The free version seems fine for what we need. Pro version pricing is per developer and there's no clarification on how that interfaces with open source projects where the number of developers is unknowable.

Is pushing data source fetching down closer to the Autocomplete and SelectFields that consume them a bad idea?

Not at all, the component that needs the data should be able to call the API to fetch it, making the component more atomic.

That's my feeling as well, but the boundary of container and component feels a little murky for elements like this.

Should reassignment actions be handled in the top level business logic hook? Or pushed down to ?

This is an interesting question. Looking at the data flow here, IncomingMessageList tells the parent what messages have been selected, and IncomingMessageActions will need to read the parents state to find what messages it should reassign. I'd say for the simpler data flow (this refactor is going to be large as is), we should let the parent manage it for now. We can come back and revisit this idea at a later time.

Sounds good! If it proves annoying I think it could work to pass selected state down to IncomingMessageActions

[hiemanshu]

Sorry, I should have clarified that I was including URL query params in my bucket of "state" - just swapping useState for useQueryParam. Agree that we shouldn't be adding more React state via useState though.

Yeah. I wonder how userQueryParams will work out.

[hiemanshu]

@kohlee @bchrobot Do you have a preference on what the URL parameters look like, these two below are common options.

Option 1 looks cleaner, but is a bit more work to define it out clearly. Option 2 works more easily, and is less work to implement and use.

[bchrobot]

Do you have a preference on what the URL parameters look like, these two below are common options.

Option 1 looks good!

Are these options based on use-query-params's Param Types ?

[hiemanshu]

Are these options based on use-query-params's Param Types ?

Option 1 uses a custom defined param type to convert the params to the correct types. So we can shorten the URL links as well using a variable name to URL name map easily as well.

Here's what a custom Param looks like:

export const ContactsFilterParam = {
    encode: (filter: ContactsFilter | undefined): string | null | undefined => {
        return encodeObject(filter);
    },
    decode: (params: string | null | undefined): ContactsFilter | null | undefined => {
        const decodedObject = decodeObject(params);

        if (!decodedObject) {
            return null;
        }

        const contactsFilter: ContactsFilter = new Object();

        for (const [key, value] of Object.entries(decodedObject)) {
            if (["isOptedOut", "validTimeZone", "includePastDue"].includes(key)) {
                contactsFilter[key] = value === "true";
            } else {
                contactsFilter[key] = value;
            }
        }
        return contactsFilter;
    }
}

[hiemanshu]

@bchrobot looked at the last required part for this, to refactor ConversationPreviewModal, and right now it seems tightly knit with the IncomingMessageList, since IncomingMessageList is the one that pulls in the conversations, and has the conversation data, I think pulling the Modal out to the main component is not ideal since it will add more complexity without much gain. So I'm going to mark this task as complete. Let me know what you think!

[bchrobot]

So I'm going to mark this task as complete. Let me know what you think!

Works for me!