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

[feature] Page through accounts as moderator #2881

Merged
merged 5 commits into from
May 1, 2024

Conversation

tsmethurst
Copy link
Contributor

@tsmethurst tsmethurst commented Apr 29, 2024

Description

If this is a code change, please include a summary of what you've coded, and link to the issue(s) it closes/implements.

If this is a documentation change, please briefly describe what you've changed and why.

This pull request implements paging through accounts alphabetically via the admin API, as opposed to paging by creation date. To accomplish this, a new index is added on a concatenation of [domain]/@[username].

Paging is also implemented in the frontend, so admins can page up and down through accounts in the settings panel.

Checklist

Please put an x inside each checkbox to indicate that you've read and followed it: [ ] -> [x]

If this is a documentation change, only the first checkbox must be filled (you can delete the others if you want).

  • I/we have read the GoToSocial contribution guidelines.
  • I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
  • I/we have not leveraged AI to create the proposed changes.
  • I/we have performed a self-review of added code.
  • I/we have written code that is legible and maintainable by others.
  • I/we have commented the added code, particularly in hard-to-understand areas.
  • I/we have made any necessary changes to documentation.
  • I/we have added tests that cover new code.
  • I/we have run tests and they pass locally with the changes.
  • I/we have run go fmt ./... and golangci-lint run.

@tsmethurst tsmethurst changed the title Pageable admin accounts [feature] Page through accounts in the settings panel Apr 29, 2024
@tsmethurst tsmethurst marked this pull request as ready for review April 29, 2024 15:19
}

if err := q.Scan(ctx, &accountIDs); err != nil {
if err := q.Scan(ctx, &accountIDs, new([]string)); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come we need this second argument? i thought we were only selecting the one column here 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are selecting account_id but also selecting domain_username as part of that query, and unfortunately bun doesn't really know what to do if you don't provide a second array to soak up the domain_username values, unless there's a way around it I haven't spotted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm wondering whether using a generated column that you also create an index on might work out a bit better (e.g. https://www.sqlite.org/gencol.html)

so generate a column name domain_username and then in both the sqlite and postgres queries can just do SELECT id FROM accounts WHERE (...) ORDER domain_username ASC/DESC. it's a column that would probably come in handy in other areas too i'm sure 🤔

@tsmethurst tsmethurst changed the title [feature] Page through accounts in the settings panel [feature] Page through accounts as moderator Apr 30, 2024
@tsmethurst
Copy link
Contributor Author

Hmm just gotta make a few fixes for postgres before squerging this.

} else {
// Create a subquery for
// Postgres to reuse.
subQ = a.db.NewRaw(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passing in this subquery multiple times in postgres, does it calculate it each and every time? or is it possible to use a temporary variable for it or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, it's computed only once because of the expression index I also added:

Index expressions are relatively expensive to maintain, because the derived expression(s) must be computed for each row insertion and non-HOT update. However, the index expressions are not recomputed during an indexed search, since they are already stored in the index. In both examples above, the system sees the query as just WHERE indexedcolumn = 'constant' and so the speed of the search is equivalent to any other simple index query. Thus, indexes on expressions are useful when retrieval speed is more important than insertion and update speed.

-- https://www.postgresql.org/docs/current/indexes-expressional.html

So I think we should be fine. I tried doing this a few different ways (using WITH, or as subqueries), but Postgres doesn't offer the same flexibility SQLite does, so this ended up being (I think) the best way of doing it, even though the queries themselves end up looking a bit silly because of the expression re-use.

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc merged commit 725a21b into main May 1, 2024
3 checks passed
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc deleted the pageable_admin_accounts branch May 1, 2024 13:11
nyarla pushed a commit to nyarla/gotosocial-modded that referenced this pull request Jun 19, 2024
* [feature] Page through accounts as moderator

* aaaaa

* use COLLATE "C" for Postgres to ensure same ordering as SQLite

* fix typo, test paging up

* don't show moderation / info for our instance acct
nyarla pushed a commit to nyarla/gotosocial-modded that referenced this pull request Jun 19, 2024
* [feature] Page through accounts as moderator

* aaaaa

* use COLLATE "C" for Postgres to ensure same ordering as SQLite

* fix typo, test paging up

* don't show moderation / info for our instance acct
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants