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

API support for storing "record preview" and "record selector" settings #944

Closed
Tracked by #451
seancolsen opened this issue Jan 5, 2022 · 27 comments
Closed
Tracked by #451
Assignees
Labels
type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Milestone

Comments

@seancolsen
Copy link
Contributor

seancolsen commented Jan 5, 2022

Description

  • To support the Record Selector and Record Preview features (for foreign key constraints), we need a way to store some settings that allow the user to customize the behavior of those features.

Motivation

Terminology

  • I'm switching terminology from "preferences" (used in the spec), to "settings" (used here) because "preferences" connotes user-specific values which I don't think we want in this case. If anyone has concerns with this adjustment, we can discuss and switch back to "preferences" language if needed.

Specs

Users read/write the settings via the API. Please note that the API paths might change based on whether we've implemented multiple API namespaces (see #1029).

/api/v0/tables/<id>/settings/

(This is a new endpoint)

  • GET response:

    {
        // this setting tracks what columns from THIS table as shown as previews in OTHER tables
        "preview_columns": {
            "columns": [278, 279, 280], // references columns by their id
            "customized": false
        }
    }
  • PATCH request

    • Same schema as GET response

    • The columns property must be specified in full.

    • All types of columns are permitted in columns.

    • Record Selector performance will be best when the columns have indexes, but we are not taking any steps to enforce that at this point.

    • The front end can reset user-customizations by sending the following request. Then columns will be re-computed.

      {
          "preview_columns": {
              "customized": false
          }
      }
    • Error if

      • columns contains any values which are not ids of columns in the table.
      • columns length is not within 1 - 5.
  • Defaults

    To create a default list for preview_columns.columns, we use the following logic to make a best-guess of what the user would want.

    • Have 5 columns if we can.
    • First, start with all columns with UNIQUE constraints that don't also have FOREIGN KEY constraints.
    • If there are fewer than 5 columns with UNIQUE constraints, fill the remaining columns with the first columns encountered that are not BOOLEAN or have FK constraints.
    • If there aren't 5 columns still, then start including BOOLEAN columns and FK-constrainted columns
  • When columns are modified

    • We should recompute columns whenever columns are added, edited, or deleted as long as customized is not set to true. This should be part of the implementation.

    • When a column is deleted, we remove any references to it within preview_columns.

    • If the user has customized the column set and there are no columns left in preview_columns after deleting a column, then we set "customized": false and re-compute the default set of columns.

/api/v0/tables/<id>/columns/

  • This endpoint already has a display_options property for each column. We need to modify the display_options property, adding a show_fk_preview property as follows:

    {
        // other column API fields go here
        "display_options": {
            // other display options go here
            "show_fk_preview": true
        }
    }
  • The show_fk_preview property will default to true if the user has not customized it. Later, once we have users, we may consider making this default into a user-specific setting.


Questions originally posed in this ticket (now answered)

All these questions have now been answered via comments, and those answers are reflected in the ticket summary above.

  • Does anyone have critique of the functionality specs that I wrote above? The official specs didn't have a whole lot of detail here so I filled in the gaps based on my own opinions.

  • Does anyone have critique of my proposed API design?

  • Defaults

    • What is the default state of use_preview_in_references?
    • What logic do we use to compute the best-guess for preview_columns?
  • How do we avoid generating a worthless default for preview_columns?

    • Here are two scenarios, that when taken together, create some interesting challenges.

      Scenario (1): User has intentionally configured preview_columns to use a single column. We'd like to preserve that setting no matter what other columns the user adds or deletes.

      Scenario (2)... User adds a blank table. It only contains an id column. We set preview_columns to use the id column automatically because we have no other choice. User adds a name column. What do we do here? Perhaps we assume that, since preview_columns contains only the PK column, the user has not customized their columns so we go ahead and set preview_columns using our best-guess logic, which incorporates name into the column list. Notwithstanding the fact that our assumption may be inaccurate, now we have a problem where preview_columns looks just like it does in scenario 1, leaving us no way to know the optimal point to compute our best guess. This situation renders our best guess logic a bit useless, leaving us only with a default of the first column created in the table. If we consider that users may want to intentionally set preview_columns equal to the PK, then we're left with a record selector that always requires configuration before it can offer any value to users who created their tables within Mathesar. Of course the situation is better for imported tables, but only initially -- when the user adds/deletes columns, the quality of the default has the potential to degrade.

    • Though complex, I wonder if it's worth adding a separate setting which tracks whether or not the user has customized the preview_columns.

      I might even consider having best_guess_preview_columns (which the API always returns, but will fail to modify) and custom_preview_columns (set to [] by default). But I'm not so sure. Others may have better ideas here.

  • Are there certain types of columns that should be disallowed from preview_columns?

    • I would suggest that for now we only allow text and numeric columns.
    • Do we want to force all preview columns to be indexed since they'll be searched within an auto-complete? I think we do. If an API consumer tries to add a non-indexed column to preview_columns, should the API throw an error? Or add an index?
  • Should use_preview_in_references be stored per-table or per-column?

    In this discussion thread, we agreed that table link preferences should be stored "per-table" (not "per-column"). I imagine that during that discussion we were all primarily considering storage of preview_columns and not giving much thought to storage of use_preview_in_references. As spec'ed above, the use_preview_in_references setting would be stored per-table too, in accordance with the consensus that emerged from our discussion. That approach is certainly the simplest to implement, but it's worth considering that users might want to show the record preview in table X and hide it in table Y, when X and Y both table Z, to which the setting is associated. As demonstrated in the prototype, the record preview can significantly reduce the number of records displayed on one screen. The specs also require a toggle for the record preview within the column header, and I can see how I'd want to have the ability to quickly toggle that setting to drill down and then zoom out. However, when placed in the column header, the setting would suggest that it's specific to the column, which I think is problematic.

    All things considered, I'd suggest that we proceed with the simplistic approach of storing the use_preview_in_references at the table level. We could consider allowing per-column storage of "show record preview" if the need becomes strong enough.

    If we want per-column control over the "Show record preview" setting now, then I'd suggest we associate that setting with single-column FK constraints. We could potentially use the constraints API to read/write the setting.

@seancolsen seancolsen added status: draft type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL labels Jan 5, 2022
@seancolsen seancolsen added this to the [08] Working with Views milestone Jan 5, 2022
@seancolsen
Copy link
Contributor Author

@kgodey @ghislaineguerin @mathemancer @silentninja @dmos62 @pavish I've assigned this ticket to you to review the questions above and comment with any thoughts. Please comment and/or unassign yourself.

@mathemancer
Copy link
Contributor

As far as the use_preview_in_references question goes, I'll abstain; seems like a UX question. Regarding the choice of preview columns:

  • You can't identify a row unless the presented tuple is unique.
  • This has a bonus effect: We'd end up with the potential to define a unique index on the preview column tuple, greatly increasing performance.

The downside is that if we go that route, it would take a bit of time to change the setting. On the other hand, the upside is that it restricts the search space for possible preview column sets, which would make suggesting simpler. As for suggestions, you point out a real problem. I'm quite partial to maintaining a separate list of user-chosen and suggested preview columns. The way would be: The user chooses some (maybe zero) preview columns, and we fill in the rest with suggestions. They could turn off the suggested preview altogether (since it may be more clutter than they want), using only their chosen columns. If they do choose columns, we'd fill in the rest, attempting to make unique tuples.

@mathemancer mathemancer removed their assignment Jan 11, 2022
@pavish
Copy link
Member

pavish commented Jan 11, 2022

@mathemancer I assume this issue was accidentally closed. I'm reopening this.

@pavish pavish reopened this Jan 11, 2022
@ghislaineguerin ghislaineguerin removed their assignment Jan 11, 2022
@ghislaineguerin
Copy link
Contributor

@seancolsen I think you make a good point in keeping preferences at the table level only. I'm wondering if record preview could be treated only as a display mode for the column, meaning it will work the same if the FK points to a different table.

@kgodey
Copy link
Contributor

kgodey commented Jan 11, 2022

Here's the schema I would suggest:

{
    // this setting tracks whether to show link previews while viewing THIS table. this may not be stored here, see the end of this comment.
    "show_link_previews": true,
    // this setting tracks what columns from THIS table as shown as previews in OTHER tables
    "preview_columns": {
        "columns": [278, 279, 280],
        "customized": false
    }
}

This namespaces the settings, allowing us to add different types of settings in the future. It also shortens setting names.

What is the default state of use_preview_in_references?

This is now show_link_previews. I think this should be true to make the product friendlier for non-technical users by default. Later, once we have users, what the default is could be a user-specific setting.

What logic do we use to compute the best-guess for preview_columns?

This is now columns. I think we should use the following logic:

  • Have 5 columns if we can.
  • First, start with all columns with UNIQUE constraints that don't also have FOREIGN KEY constraints.
  • If there are less than 5 columns with UNIQUE constraints, fill the remaining columns with the first columns encountered that are not BOOLEAN or have FK constraints.
  • If there aren't 5 columns still, then start including BOOLEAN columns and FK-constrainted columns

How do we avoid generating a worthless default for preview_columns

We should recompute columns whenever columns are added, edited, or removed as long as customized is not set to true. This should be part of the implementation.

Are there certain types of columns that should be disallowed from preview_columns?

No, we'd be making too many assumptions to exclude any columns, I think. Dates, emails, URLs, etc. can all be unique.

Also, the columns are not only used to search, they're also used to see a preview of the related record. For example, see:

ID Actor Movie ID
24 Brad Pitt 2
34 Henry Cavill 3
ID Movie Title Watched
2 Meet Joe Black TRUE
3 Enola Holmes FALSE

In this situation, I might want to put "Watched" in the preview columns. Even though it won't help me search, it's nice to see whether I've watched a movie or not from the preview instead of having to navigate to the other table.

Do we want to force all preview columns to be indexed since they'll be searched within an auto-complete? I think we do. If an API consumer tries to add a non-indexed column to preview_columns, should the API throw an error? Or add an index?

No, it requires too much DB knowledge to have indexes set up. We can open a separate issue to track performance issues with auto-complete if they arise. I don't think we want to automatically add indexes as part of the issue, because it'll impact write performance and we don't want to make assumptions that that's okay – the user could be using the databases in non-Mathesar applications where that will be a problem.

Should use_preview_in_references be stored per-table or per-column?

I was originally envisioning it as per-column. I thought it would be an addition to the Columns API (show_link_previews can be part of the column display options) and we would automatically set it to null if there wasn't a single-column FK set on the column.

I think this will offer the most flexibility to the user (they can expand some FKs and hide others). I'm not sure why having the setting associated with the column is problematic.


@seancolsen I'll leave it to you to update the body of this ticket since you didn't specifically assign it to me. Let me know if I can help, though.

@kgodey kgodey assigned seancolsen and unassigned kgodey Jan 11, 2022
@seancolsen
Copy link
Contributor Author

Auto-creating indexes

I said:

Do we want to force all preview columns to be indexed? I think we do.

@mathemancer said:

We'd end up with the potential to define a unique index on the preview column tuple

But @kgodey said:

I don't think we want to automatically add indexes

@kgodey has convinced me that we should not automatically create indexes. And I think we can focus on this topic later when we design Mathesar's indexing features.

Choosing default preview columns

@mathemancer said:

The user chooses some (maybe zero) preview columns, and we fill in the rest with suggestions

I'm having a bit of an intuitive knee-jerk reaction against this idea, but I'm not yet able to fully rationalize it. If others are excited to move in this direction I'll work harder on articulating my concerns. For now I'll sum it up by saying that I'm not sold on the value that this additional complexity adds.

Where to store the preview toggle

@ghislaineguerin said:

I'm wondering if record preview could be treated only as a display mode for the column

@kgodey said:

I was originally envisioning it as per-column

That seems like enough of a consensus thus far, and I agree that per-column would make the most sense from a UX perspective.

However, @kgodey your proposed schema doesn't seem to store it per-column.

  1. Say I have the following schema:

    - movie
      - id
      - title
    
    - actor
      - id
      - name
    
    - role
      - id
      - movie_id (FK to movie.id)
      - actor_id (FK to actor.id)
      - character_name
  2. "Per-column" would mean that in the role table, I can have the movie preview enabled while having the actor preview disabled. However with your schema, the visibility of all the previews displayed within one table would be bound together.

Then @kgodey said:

I thought it would be an addition to the Columns API (show_link_previews can be part of the column display options) and we would automatically set it to null if there wasn't a single-column FK set on the column.

That sounds better. Are you saying show_link_previews would be accessible in the tables settings API and the columns API?

What do you think about putting it in the constraints API instead? Each FK constraint would have an associated show_link_previews boolean value. No need for null anywhere. Multi-column FK constraints would always return show_link_previews: false, and (for the time being) attempting to set them to true would result in an error.

Other misc reactions

@kgodey I agree with everything you said regarding:

  • Schema for preview_columns.columns and preview_columns.customized
  • Showing previews by default
  • Algorithm to choose the default set of preview columns
  • Logic for deciding when to recompute the default preview columns
  • All columns being allowed as preview columns

@kgodey I'll update the ticket description once more people have weighed in and we've reached a rough consensus about all the details here.

@kgodey
Copy link
Contributor

kgodey commented Jan 12, 2022

@seancolsen Sorry for the confusion, I was responding to each of your questions in turn and not all together. I think the actual schema should be:

/api/v0/tables/<id>/settings/

{
    // this setting tracks what columns from THIS table as shown as previews in OTHER tables
    "preview_columns": {
        "columns": [278, 279, 280],
        "customized": false
    }
}

/api/v0/tables/<id>/columns/

{
    // other column API fields go here
    "display_options": {
        // other display options go here
        "show_fk_preview": true
    }
}

@kgodey
Copy link
Contributor

kgodey commented Jan 12, 2022

I think it would be better to have it in the columns API since it's a display setting of a column and we already have a display_options provision for columns. That way, every formatting choice related to columns is in one place.

@pavish
Copy link
Member

pavish commented Jan 12, 2022

I'm onboard with @kgodey's suggestions.

Regarding this comment from @mathemancer:
The user chooses some (maybe zero) preview columns, and we fill in the rest with suggestions.

I don't think we should override user selections. The user might actually want to only preview a single column, for eg., only select the email from the referenced table and nothing else. If we auto fill the rest, we only add more clutter. On the other hand, we should restrict the user to atleast select one column (never zero).

Edit: I see that this is mentioned on the issue description and is the reasoning behind the customized setting and I am onboard with it.

@kgodey
Copy link
Contributor

kgodey commented Jan 12, 2022

Note: I revised my schema in #944 (comment) to change the setting name to show_fk_preview to be consistent with the query parameter name in #946.

@silentninja
Copy link
Contributor

silentninja commented Jan 14, 2022

If many-to-many then the record preview will show the records belonging to the mapping table, in which case the design doesn't change

@ghislaineguerin The user would actually be trying to search the column the mapping table refers to, so should we have provision to search through the referred column of mapping table or point them to visit the mapped table instead

@silentninja
Copy link
Contributor

We'd start by finding any unique constraints, and then suggest columns from unique constraints where the user has already chosen some of the involved columns

@mathemancer This is a bit confusing. You mean to suggest we find a unique row pair even though the column itself is not unique but when combined could be unique, is that right?

@kgodey
Copy link
Contributor

kgodey commented Jan 14, 2022

we might consider a hepful infobox extolling the benefits of uniqueness constraints if they've chosen poorly.

@mathemancer I recommend opening a new design issue to consider this after the alpha release so we don't forget about it. It is not part of the current spec.

I do think we should choose the suggested columns to help fill out a unique set, in combination with the user's selected columns when possible.

I like this addition to the algorithm for choosing suggested columns, I will leave it to @seancolsen to make the necessary updates to the issue.

Side note: @ghislaineguerin @kgodey I looked back through the spec, and couldn't figure out whether we intend to support previews for many-to-many or one-to-many (i.e., previewing the backwards direction along a foreign key reference). Do we need that?

@ghislaineguerin The user would actually be trying to search the column the mapping table refers to, so should we have provision to search through the referred column of mapping table or point them to visit the mapped table instead

We will not support this per the current spec, the user will have to add FKs in the mapping table directly. We can consider adding functionality for searching "through" mapping tables after the alpha release.

@silentninja silentninja removed their assignment Jan 14, 2022
@kgodey kgodey modified the milestones: [09] Working with Views, [08] Links Between Tables Jan 18, 2022
@mathemancer
Copy link
Contributor

@mathemancer This is a bit confusing. You mean to suggest we find a unique row pair even though the column itself is not unique but when combined could be unique, is that right?

Yes, exactly.

@seancolsen
Copy link
Contributor Author

I opened #1039 to splinter off the feature of suggesting additional columns to users during the column customization process.

@seancolsen seancolsen added ready Ready for implementation and removed status: draft labels Feb 2, 2022
@seancolsen
Copy link
Contributor Author

I've updated the description to reflect resolution of all the above discussion and marked this as ready for implementation now. We also had a brief discussion in Matrix about this today, making sure everyone is copacetic.

@kgodey
Copy link
Contributor

kgodey commented Feb 2, 2022

@seancolsen I made a small update about the API paths, right under the "Specs" heading.

@silentninja
Copy link
Contributor

@dmos62 Can you remove your assignment if you finished sharing your feedback on this issue?

@dmos62 dmos62 removed their assignment Feb 9, 2022
@silentninja silentninja self-assigned this May 18, 2022
@silentninja silentninja added status: started and removed ready Ready for implementation labels May 18, 2022
@silentninja
Copy link
Contributor

silentninja commented May 20, 2022

I am having issues with the spec'ed out API schema.

  • Is there a reason why we need a separate route for /table/{id}/settings/ since there is only one setting object for a table? Shouldn't settings be part of the table object?
  • How would the frontend get the settings id to send a PATCH request to update settings.

But having a separate route will make sense if we have user settings which would be implemented in future, so I am confused on which API schema to choose

@silentninja
Copy link
Contributor

silentninja commented May 20, 2022

I had a call with @seancolsen, the options we came up with were

Option 1(Spec'ed schema)

Not a valid Restful API as the GET request was using an object instead of an array, option 3 listed below is an alternate API that is restful.

Option 2 (part of tables API)

GET /api/v0/tables/<id>/

{
  // ...
  "preview_columns": {
    "columns": [278, 279, 280],
    "customized": false
  }
}

Option 3 (separate entity)

GET /api/v0/tables/<id>/settings/

[
  {
    "id": 1287,
    "preview_columns": {
      "columns": [278, 279, 280],
      "customized": false
    }
  }
]

GET /api/v0/tables/<id>/settings/<id>

{
  "id": 1287,
  "preview_columns": {
    "columns": [278, 279, 280],
    "customized": false
  }
}

PATCH /api/v0/tables/<id>/settings/<id>

{
  "preview_columns": {
    "columns": [278, 279, 280],
    "customized": false
  }
}

We decided to go with Option 3 as it was easier to implement on the backend as well as giving us the option to extend it in the future to support user settings

@silentninja
Copy link
Contributor

Partially closed by #1391 and the remaining issues are tacked by #1461

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

No branches or pull requests

7 participants