-
-
Notifications
You must be signed in to change notification settings - Fork 457
Experiment: Add a "User" data type #4997
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
base: develop
Are you sure you want to change the base?
Conversation
- Add user_type metadata field to ColumnMetadata model - Add user type column handling in RPC endpoints (columns, records, forms) - Add user display utilities for formatting user data - Add migration for user_type column metadata - Make user_display_field nullable to match pattern of other type-specific fields
- Add UserCell, UserInput, and UserFilterInput components - Add user type to abstract type system and type definitions - Add user type column display options (username, display name, email) - Add user type filtering support - Add user type default value options (auto-set to editor, default user) - Prevent user type columns from being added to forms - Add user utilities for fetching and displaying users
- Add comprehensive user type guide with examples - Add user type images for documentation - Update data types documentation to include user type
|
@mathemancer please review the backend implementation in Regarding the frontend, I need to compare this to some overlapping work in cross-table editing to validate the approach, and make this a bit easier to review for the frontend team before getting frontend review. |
mathemancer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review so you have stuff to think about:
- Please avoid hiding the mutation of python objects in function calls, as you do in
_add_user_display_values_to_record_infoet al. I'd really prefer the modifications be inline where they're clearly visible; see the addition ofdownload_linksfor example. - You need to refactor this so that a given RPC call never generates multiple requests for the same Django model object. For example,
records.addcallsget_columns_meta_datain both_set_last_edited_by_columnsand_add_user_display_values_to_record_info. The problem is also present inrecords.listsince bothget_download_link_columnsand_add_user_display_values_to_record_infocall theget_columns_meta_datafunction. We either need to cache that metadata getting function for the duration of a request, or (easier) just call it in the mainrpcfunction body then use it where needed.
More to come later; late for the team event!
| max_length=50, | ||
| null=True | ||
| ) | ||
| user_last_edited_by = models.BooleanField(default=False, null=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this name really confusing. What about track_editing_user?
| duration_min: Optional[str] | ||
| duration_max: Optional[str] | ||
| display_width: Optional[int] | ||
| file_backend: Optional[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, nice catch
| display_width: Optional[int] | ||
| file_backend: Optional[str] | ||
| user_type: Optional[bool] | ||
| user_display_field: Optional[Literal["full_name", "email", "username"]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If user_display_field is optional, I guess I don't see why user_type is needed. If this is non-null, then display the column as specified, otherwise, just display as an integer.
If you really really want to keep user_type, I don't think that should be optional since it's always either true or false. Just use false for the default.
| file_backend=model.file_backend, | ||
| user_type=model.user_type, | ||
| user_display_field=model.user_display_field or "username", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think user_last_edited_by is missing here.
| from modernrpc.core import REQUEST_KEY | ||
| request = kwargs.get(REQUEST_KEY) | ||
| user = request.user if request else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why are you reimporting
REQUEST_KEY? - This isn't going to be called without a request, right? If not, there's way too much logic here.
| table_oid: int, | ||
| database_id: int, | ||
| limit: int = None, | ||
| offset: int = None, | ||
| order: list[OrderBy] = None, | ||
| filter: Filter = None, | ||
| grouping: Grouping = None, | ||
| return_record_summaries: bool = False, | ||
| **kwargs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like your editor really hates the double-indent function args style, but I prefer it. This comment is applicable to many of your PRs (maybe all)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll disable automatic formatting in vscode for Mathesar. Preferences aside it clutters the diffs too much anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll disable automatic formatting in vscode for Mathesar. Preferences aside it clutters the diffs too much anyway.
| """ | ||
| user = kwargs.get(REQUEST_KEY).user | ||
| # Automatically set user_last_edited_by columns to current user ID | ||
| _set_last_edited_by_columns(record_def, table_oid, database_id, user.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in a separate module, and should not modify records in place.
| or None | ||
| ) | ||
|
|
||
| _add_user_display_values_to_record_info(record_info, table_oid, database_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in a separate module, and should not modify in place.
This experimental PR adds a user data type to Mathesar. This PR should be considered a proof-of-concept but also contains code sufficient for review and production use. We may wish to split this into multiple PRs for easier review, when we're ready. It's already split by commits, one for:
I'm publishing this draft now so an interested user can review this feature and provide feedback. This is not ready for full review by the team.
This PR will be updated with a more detailed technical description when it is ready for full review.
Checklist
Update index.md).developbranch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin