-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add "My series" page #1311
base: next
Are you sure you want to change the base?
Add "My series" page #1311
Conversation
0d8826c
to
7b97115
Compare
40bae15
to
7a054e4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
7768227
to
2582293
Compare
This comment was marked as outdated.
This comment was marked as outdated.
2582293
to
46c24bc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
46c24bc
to
53908b1
Compare
b6b8bca
to
8d766f6
Compare
8d766f6
to
1e32aed
Compare
This comment was marked as outdated.
This comment was marked as outdated.
1e32aed
to
19ebb5d
Compare
a9b6065
to
618ba48
Compare
This generalizes and factors out some backend code so it can also be used for series and playlists. It also replaces the cursor based pagination with an offset based one. That simplifies a lot of things and allows us to get rid of a bunch of code. The frontend now uses a `page` url parameter to signify the current page. When entered manually, there are some checks and logic to make sure it's always in bounds, i.e. using a number smaller than 0 will always redirect to `page=1` and a number larger that the max number of pages will redirect to the last page. Please note that the generalization of the sorting columns will be done in a later commit.
This factors out most of the table code for re-use with other assets, namely series and playlists. Please note that the generalization of column sorting and display will be done in later commits.
This includes: (a) Backend code that allows generating custom enums for different sorting columns, trying to minimize the necessity of code duplication (b) Frontend code mostly related to parsing the custom sorting columns from URL parameters and passing these to the API Generalization of the table, to represent these custom columns will be done in the next commit.
This will now take any custom columns into consideration and makes their declaration easier while trying to limit duplicated code as much as possible without adding too much complexity. Tables now have a __somewhat__ clear and fixed stucture. Any customization, including additional columns, can be done in the respective `index` files of the asset. This is assuming that all asset tables have thumbnails, titles and descriptions that share at least some portion of their styling. But they can also be further customized. All of this is also in preparation for adding a playlist table later on. Adding that should be pretty straightforward now.
Todo: delete the other commit.
This combines two things: (a) It fixes the event query for the `My videos` table. Adjumstments became necessary after some upstream changes to the series column in the `from_db` impl of authorized events. (b) It moves some table specific query customizations from the generic `load_writable_for_user` function to the impls of the `LoadableAsset` trait.
I forgot the fact that the table might include events that have been marked as deleted. Unfortunately that means the query and `LoadableAsset` trait are getting increasingly complicated to the point where it's hard to understand and quite easy to make these errors. This is also in part due to the series table's need to be sortable by event count, and I am starting to wonder if it might be better to move the whole query to the trait, though that brings its own complications and I doubt that would even be possible without other major adjustments.
618ba48
to
14da37c
Compare
I didn't like the name.
It did not need to be so complicated.
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 is just a review of the backend code. And while I left quite a lot of inline comments, I am actually very happy with that! The approach is sane and you used Rust features cleverly, even wrote a macro! Most of my comments just refer to minor things one can write in a shorter way.
pub(crate) struct EventConnection { | ||
page_info: EventPageInfo, | ||
items: Vec<AuthorizedEvent>, | ||
total_count: i32, | ||
} | ||
|
||
#[derive(Debug, Clone, Serialize, Deserialize)] | ||
pub(crate) struct EventCursor { | ||
key: Key, | ||
sort_filter: CursorSortFilter, | ||
} | ||
|
||
#[derive(Debug, Clone, Serialize, Deserialize)] | ||
enum CursorSortFilter { | ||
Title(String), | ||
Duration(Option<i64>), | ||
Created(DateTime<Utc>), | ||
Updated(Option<DateTime<Utc>>), | ||
inner: Connection<AuthorizedEvent>, | ||
} | ||
|
||
impl EventCursor { | ||
fn new(event: &AuthorizedEvent, order: &EventSortOrder) -> Self { | ||
let sort_filter = match order.column { | ||
EventSortColumn::Title => CursorSortFilter::Title(event.title.clone()), | ||
EventSortColumn::Created => CursorSortFilter::Created(event.created), | ||
EventSortColumn::Updated => CursorSortFilter::Updated( | ||
event.synced_data.as_ref().map(|s| s.updated) | ||
), | ||
}; | ||
|
||
Self { | ||
sort_filter, | ||
key: event.key, | ||
} | ||
#[graphql_object(context = Context)] | ||
impl EventConnection { | ||
fn page_info(&self) -> &PageInfo { | ||
&self.inner.page_info | ||
} | ||
|
||
/// Returns the actual filter value as trait object if `self.sort_filter` | ||
/// matches `order.column` (both about the same column). Returns an error | ||
/// otherwise. | ||
fn to_sql_arg(&self, order: &EventSortOrder) -> ApiResult<&(dyn ToSql + Sync + '_)> { | ||
match (&self.sort_filter, order.column) { | ||
(CursorSortFilter::Title(title), EventSortColumn::Title) => Ok(title), | ||
(CursorSortFilter::Created(created), EventSortColumn::Created) => Ok(created), | ||
(CursorSortFilter::Updated(updated), EventSortColumn::Updated) => { | ||
match updated { | ||
Some(updated) => Ok(updated), | ||
None => Ok(&postgres_types::Timestamp::<DateTime<Utc>>::NegInfinity), | ||
} | ||
}, | ||
_ => Err(invalid_input!("sort order does not match 'before'/'after' argument")), | ||
} | ||
fn items(&self) -> &Vec<AuthorizedEvent> { | ||
&self.inner.items | ||
} | ||
fn total_count(&self) -> i32 { | ||
self.inner.total_count |
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.
You can also do the following to avoid the extra struct
and inner
field:
#[graphql_object(name = "EventConnection", context = Context)]
impl Connection<AuthorizedEvent> {
fn page_info(&self) -> &PageInfo {
&self.page_info
}
...
}
We do something similar in api/model/search/mod.rs
for EventSearchResults
and co.
}, | ||
_ => Err(invalid_input!("sort order does not match 'before'/'after' argument")), | ||
} | ||
fn items(&self) -> &Vec<AuthorizedEvent> { |
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.
Could also be -> &[AuthorizedEvent]
I guess, but 🤷
#[derive(Debug, Clone, GraphQLObject)] | ||
pub struct PageInfo { | ||
pub has_next_page: bool, | ||
pub has_previous_page: bool, |
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 am a big fan of abbreviating previous
with prev
because then it's the same length as next
and things line up nicely 😬
Change if you agree, otherwise it's fine to keep it that way :D
pub start_index: Option<i32>, | ||
pub end_index: Option<i32>, |
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.
Without having read a lot of this PR already, I'm not clear on what these fields represent. I assume its the index of the first/first element on that page. And the index according to the sort order? Why is it Option
? Given that I have that many questions, maybe the two fields should have a short doc comment explaining that? Or a doc comment on PageInfo
itself to avoid duplicating some info.
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.
Ah I didn't add these, they were carried over from the cursor based pagination. Now they are only used to show these numbers (first/last element of page, as you suspected) for the "next" and "previous" buttons. I suppose we could also calculate these numbers from the offset- and limit parameters directly in frontend (right now that is done in backend).
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.
Mh oh true... I suppose offset
to offset + min(limit, items.length)
or sth like that should always be correct? Yeah why not remove it from the API then 🤷
pub end_index: Option<i32>, | ||
} | ||
|
||
pub type ItemMapping<ResourceMapping> = ResourceMapping; |
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.
Wow, what is this funky type? Couldn't any usage of it, e.g. ItemMapping<Foo>
always be replaced by just Foo
?
impl Default for $enum_name { | ||
fn default() -> Self { | ||
Self::$default | ||
} | ||
} |
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.
There is actually a small little feature of #[derive(Default)]
for enums. This works and does what you think:
#[derive(Debug, Default)]
enum Animal {
Cat,
Dog,
#[default]
Fox,
}
It's not that important, since you have a macro anyway, but maybe this simplifies some stuff. You could change the input grammar to sth like:
$vis_enum:vis enum $enum_name:ident {
$( $(#[default])? $variant:ident => $sql:expr),+ $(,)?
};
And the expansion inside enum
would be sth like:
$( $(#[default])? $variant),+
Didn't test it, but should work roughly like that.
pub trait ToSqlColumn { | ||
fn to_sql(&self) -> &'static 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.
Could use a tiny bit of docs
#[derive(Debug, Clone, Copy, PartialEq, Eq, GraphQLEnum)] | ||
pub enum SortDirection { | ||
Ascending, | ||
Descending, | ||
} | ||
|
||
impl SortDirection { | ||
pub fn to_sql(self) -> &'static str { | ||
match self { | ||
SortDirection::Ascending => "asc", | ||
SortDirection::Descending => "desc", | ||
} | ||
} | ||
} |
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 would probably move that up, next to struct SortOrder
#[graphql(default)] | ||
order: Option<VideosSortOrder>, | ||
offset: i32, | ||
limit: i32, | ||
) -> ApiResult<EventConnection> { | ||
AuthorizedEvent::load_writable_for_user(context, order, first, after, last, before).await | ||
let order = order.unwrap_or(VideosSortOrder { | ||
column: VideosSortColumn::Created, | ||
direction: SortDirection::Descending, | ||
}); | ||
AuthorizedEvent::load_writable_for_user(context, SortOrder { | ||
column: order.column, | ||
direction: order.direction | ||
}, offset, limit).await | ||
} |
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.
Mh it seems like the Default
impl of VideosSortOrder
is not used at all? This argument order
defaults to None
and you unwrap_or
manually below. You could add impl<C: Default> Default for SortOrder<C> { .. }
. Though mh that means Descending
is used for all orders, regardless of C
. You can also add two impls ala impl Default for SortOrder<VideosSortColumn>
? And then remove the Option
here. With a previous comment of removing VideosSortOrder
, this should work in the end:
async fn my_videos(
&self,
context: &Context,
#[graphql(default)]
order: SortOrder<VideosSortColumn>,
offset: i32,
limit: i32,
) -> ApiResult<Connection<AuthorizedEvent>> {
AuthorizedEvent::load_writable_for_user(context, order, offset, limit).await
}
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 had to take a slightly different approach since we still need the VideosSortOrder
struct. So instead of the above suggestion, I added Default
and From<...>
impls for the generated order structs to the macro.
updated: "case \ | ||
when ${table:series}.updated = '-infinity' then null \ | ||
else ${table:series}.updated \ | ||
end", |
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.
There is nothing wrong with this solution, but just to show you an alternative: you can say row.updated::<postgres_types::Timestamp::<DateTime<Utc>>>()
and then match on that. But it probably doesn't even save code...
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.
Interesting. I tried something along those lines but couldn't get it working, though I image it shouldn't be that hard. But if you don't mind the current version, I'll just leave it at that.
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.
Ok, here is a review of the frontend. I didn't test it in detail yet, but from what I saw, I like it. Looks good.
sortBy !== null ? match<string, SeriesSortColumn>(sortBy, { | ||
"title": () => "TITLE", | ||
"created": () => "CREATED", | ||
"updated": () => "UPDATED", | ||
"event_count": () => "EVENT_COUNT", | ||
}) : "CREATED"; |
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.
Mhh somehow type safety failed us. If you specify a garbage value as this parameter, the whole frontend crashes. The problem is that match
does not get a third "default" parameter, so it should return T | null
. Ah mh I think the typing of match
is just wrong. Very related to the Record
problem I recently mentioned in chat. Anyway, the solution for Tobira is to just add a third parameter "CREATED"
to the match
call. I will take care of fixing match
.
pub start_index: Option<i32>, | ||
pub end_index: Option<i32>, |
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.
Mh oh true... I suppose offset
to offset + min(limit, items.length)
or sth like that should always be correct? Yeah why not remove it from the API then 🤷
mySeries(order: $order, offset: $offset, limit: $limit) { | ||
__typename | ||
totalCount | ||
pageInfo { | ||
hasNextPage hasPreviousPage | ||
startIndex endIndex | ||
} | ||
items { | ||
id | ||
title | ||
created | ||
updated | ||
syncedData { description } | ||
entries { | ||
__typename | ||
...on AuthorizedEvent { | ||
isLive | ||
syncedData { thumbnail audioOnly } | ||
} | ||
} | ||
} |
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 am sensing an n+1 query problem. Worse even n*m+1. I bet the backend has to execute an SQL query for every single video of every series listed.
Mhh.. how to fix that though. So we only need the video count and three thumbnail infos to render the stack. I guess we need to add two new fields to Series
, namely eventCount
and thumbnailStackInfo
or sth like that. Aand then we need to adjust some SQL queries to make sure we don't need to execute many. Ideally, this whole mySeries
should only send a single SQL query... mh.
EDIT: ok i tested it now and it's not an n*m+1 problem, "just" n+1. All videos and all their infos of one series are retrieved in one query. So that's "just" as many queries as entries in the table. Mhh I can have a look later how one might easily optimize that. But maybe first solve everything else.
// Seems odd, but simply checking `e => e.__typename === "AuthorizedEvent"` will produce | ||
// TS2339 errors when compiling. | ||
type AuthorizedEvent = Extract<Entry, { __typename: "AuthorizedEvent" }>; | ||
const isAuthorizedEvent = (e: Entry): e is AuthorizedEvent => | ||
e.__typename === "AuthorizedEvent"; | ||
|
||
const thumbnails = series.entries | ||
.filter(isAuthorizedEvent) |
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, the old filter
problem. You can inline the function though, if you want. And the comment could be removed as well I think, we have multiple cases like this in the codebase.
// Seems odd, but simply checking `e => e.__typename === "AuthorizedEvent"` will produce | |
// TS2339 errors when compiling. | |
type AuthorizedEvent = Extract<Entry, { __typename: "AuthorizedEvent" }>; | |
const isAuthorizedEvent = (e: Entry): e is AuthorizedEvent => | |
e.__typename === "AuthorizedEvent"; | |
const thumbnails = series.entries | |
.filter(isAuthorizedEvent) | |
type AuthorizedEvent = Extract<Entry, { __typename: "AuthorizedEvent" }>; | |
const thumbnails = series.entries | |
.filter((e): e is AuthorizedEvent => e.__typename === "AuthorizedEvent") |
if (limit !== LIMIT) { | ||
searchParams.set("limit", String(limit)); | ||
} |
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.
Mh so random piece of code to talk about that but: do we really need the limit
URL parameter? The UI currently offers no way to change the "items per page". So unless you manually edit the URL this limit will always be our fixed limit 15. Did you plan on adding "items per page" selector in the UI?
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.
Uh no, just thought it'd be nice to have 😄.
I suppose we could add an UI selector, or get rid of that parameter. I'm fine with both.
Edit: Got rid of it.
const PageNavigation: React.FC<SharedProps> = ({ connection, vars }) => { | ||
const { t } = useTranslation(); | ||
const pageInfo = connection.pageInfo; | ||
const total = connection.totalCount; | ||
|
||
const limit = vars.limit ?? LIMIT; | ||
const offset = vars.offset ?? 0; | ||
|
||
const prevOffset = Math.max(0, offset - limit); | ||
const nextOffset = offset + limit; | ||
const lastOffset = total > 0 | ||
? Math.floor((total - 1) / limit) * limit | ||
: 0; | ||
|
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.
Seems a bit roundabout to me. Maybe thats a remnant from the previous code but why not just pass the current page
to this component. Then you don't have to do as much calculation and stuff. I suppose the whole frontend hardly needs to know about offset
. The offset only needs to be calculated temporarily to send to the API, but I think everywhere else I would just use page
. So that also means not passing these GraphQL vars to <ManageItems />
, but just the "query vars", i.e. page
, sort order and potentially the limit
if we really want to keep that dynamic.
if (page > maxPage) { | ||
window.location.href = `?page=${maxPage}`; | ||
} else if (page < 1) { | ||
window.location.href = "?page=1"; | ||
} | ||
}, [page, totalCount, limit]); |
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.
Setting window.location
is a hard reload. Have you tried using const { router } = useRouter();
and router.replace(...)
.
{connection.__typename === "EventConnection" && connection.items.map(event => | ||
<EventRow key={event.id} event={event} />) | ||
} | ||
{connection.__typename === "SeriesConnection" && connection.items.map(series => | ||
<SeriesRow key={series.id} series={series} />) | ||
} |
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.
Mhh I don't think that's too pretty. For one, it reads weird query response things, but more importantly, the data flow between components and files is a bit weird. Both "callers" of <ManageItems>
pass all data to that component, but then randomly, the component deep inside imports another component from the "calling" file. I think it would be better if the series/event route would pass this component to ManageItems
. So add a RenderRow: (item: T) => ReactNode;
prop to ManageItem
. Then it needs to be generic, yes, and also some of the helper sub-components. But I think this will also actually add nice typing to lots of components. No more type SortColumn = VideosSortColumn | SeriesSortColumn;
and the like. At this place you would just do <RenderRow ... />
.
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 knew I forgot something that I still wanted to change. This is it. I completely agree with what you're saying here. It's not pretty, at all.
notReadyLabel: "series.not-ready.label", | ||
}} | ||
customColumns={seriesColumns.map(col => ( | ||
<Fragment key={col.key}>{col.column(series)}</Fragment> |
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 rather be <col.column ...>
and changing the column
type to ({ item: ... }) => ReactNode
. Or ComponentType<{ item: ... }>
. The point is that we want to render it using the normal JSX react magic. Sometimes its useful to have a simple function that returns JSX and to call it directly. But things can go wrong, mostly related to hooks.
For example, in some columns <DateColumn>
is used, which in turn uses many hooks. When not rendering with JSX, but calling a function, these hooks are counted as part of the parent component, which leads to the typical problems with hooks: they always need to be called in the exact same order for each component, unconditionally. Apparently this doesn't lead to crash here but it's close. So yeah, treat column
as a component.
This adds a page to the "manage pages" area were users can see a list of their series.
Most of these changes are related to generalizing the "My videos" backend and frontend code, so it can also be used for series-, and later on playlist management.
This PR also addresses two issues related to the "My videos" table.
Please note that this is only the first step in adding a proper series management in Tobira. Right now, the list entries link directly to the corresponding series. This will be changed later to link to a "Series details" page.
Note for the reviewer: This is probably better to review file-by-file, since a lot of the commits touch and revise the same files.
Part of #355
Fixes #759
Closes #527