Allow admins to resolve removed or deleted objects via API

[Nothing4You]

No description provided.

[dessalines]

We don't have this style anywhere in the codebase that I can find. Usually we put the tests in the same file as the functions needed to be tested.

[Nothing4You]

it gets ugly when you have common logic that is used in multiple files with test cases, which is why i moved it out of that. i've done the same thing for crates/api/src/site/registration_applications btw.

[dessalines]

Rather than create a new struct and impl, I'd move this to crates/db_views/src/local_user_view.rs : and do

impl LocalUserView {
  fn create_test_user(..

[Nothing4You]

probably makes sense to have it over there anyway so it can be used by other tests as well

[Nothing4You]

wouldn't changing this to an impl for LocalUserView prevent using the TestUser struct with defaults for the creation? how would this have to be written?

[dessalines]

You can use whatever params you like to create the LocalUserView. There's no need to have a special struct for a test user, especially if all you're checking is 1 or 2 fields.

impl LocalUserView {
  pub async fn create_test_user(self, pool, ..whatever_other_params, ) -> LemmyResult<Self> {
    let instance_id = Instance::
    ...
  }
}

[Nothing4You]

which in turn means there is no default(), and all params must always be provided, making this less usable again.
the function currently creates a Person and then a LocalUser. there are many places in the code where tests are setting more than just the name, bio and admin status. for this to work properly for all those configurations it doesn't make sense to build a list of 20+ arguments for LocaluserView::create_test_user().
if it's not that generic there is no point in moving it over to LocalUserView in the first place, as most tests will have different requirements in what needs to be set on the user.

even in just the two files touched in this PR, this already sets the admin flag in one test and the bio in the other test. looking at crates/db_schema/src/impls/local_user.rs, this sets the email of the LocalUser. crates/db_views/src/post_view.rs sets the bot_account flag of the Person.

if this is moved to a more central location with the intention of calling it from other tests it'll have to be able to take all the parameters used by the different tests. i don't intend to add support for more parameters than these two in this PR to keep the PR more readable, but i could create another PR afterwards that will change other tests to also use the same common test user creation function.

[dessalines]

You don't need to provide all the params, I'm not sure what's confusing.

impl LocalUserView { 
  pub async fn create_test_user(self, pool, INSERT_ALL_PARAMS_YOU_NEED_HERE ) -> LemmyResult<Self> { 
    // Move things from line 21 into here
  } 
}

[Nothing4You]

Yes you do, see my previous comment.

If the function is supposed to be usable in all the different tests across different crates and modules it'll have to accept all the different inputs they require, otherwise it's not really improving the situation if you still have to update the person/local_user after the creation to get it in the state needed in those tests.
The only way I can see to achieve that is to add a bunch of Options that you'll always have to pass None for in all cases where they are not needed.

The overall idea is to reduce boilerplate and duplicate code, which should also help avoid mistakes when creating objects during testing and improve readability by reducing the lines of code required in tests.

The intention of the change in this PR was to use a common method to both create a Person and also create the associated LocalUser. When you use SomeInsertForm, you have the option of writing it like this:

let local_user_form = LocalUserInsertForm {
  admin: Some(true),
  ..LocalUserInsertForm::test_form(inserted_person.id)
};

This however does not allow you to simplify the creating of a Person and a LocalUser at once for tests, as you will always have to explicitly create both forms.
I don't think it makes sense to put a bunch of unrelated refactoring into this PR, so I'm also fine with just reverting this to having just a local creation function in the test module of each of the affected files in here.

For the actual refactoring, which would be in a separate PR, what do you think about adding a submit(pool) method to forms as first step in that direction?

impl PersonInsertForm {
  pub async fn submit(&self, pool: &mut DbPool<'_>) -> Result<Person, Error> {
    Person::create(pool, self).await
  }
}

See Nothing4You@3ed0879 for what this could look like in practice.
Do you think it would make sense to refactor this for all the forms and also use it outside of the tests?

[Nutomic]

.submit(pool) does look much cleaner than the current code.

[dessalines]

submit might be a pointless wrapper, and maybe a little confusing. For example, for the PostSaved form, we have a Saveable trait, so you can do save and unsave (one which inserts, one which deletes the row) Adding submit wouldn't be clear which one it needs to do.

Same goes for all the Crud trait functions. How do you know which operation submit is performing?

@Nothing4You here's what i mean, you can eliminate the pointless testuser and all those new imports by just moving the test user creation to the object impl itself: Nothing4You#5

[Nutomic]

Of course there should only be one way to run a specific query, not multiple.

Regarding your create_test_user(), the problem is that every caller needs to specify bio and admin, even if they are completely irrelevant. It still works in this example because there are only two extra params, but if its used across the whole codebase there would be many more params which are only needed a single time. Then its much easier to omit unused params using struct init with ..Default::default().

Though it would be better to make a separate PR for refactoring only.

[Sjmarf]

Is there a reason why everyone isn't allowed to resolve removed comments? Removed comments are still returned by the API when you query the comment endpoint, but they can't be resolved directly with resolve_object

[Nutomic]

@Sjmarf If removed comments are returned when youre not a mod/admin thats a bug. Are you using the latest release?

[Sjmarf]

@Nutomic

I tried it on sh.itjust.works (0.19.5) and lemmy.ml (0.19.6-beta1). On both instances I can use the comment endpoint to get a removed comment like so:

https://sh.itjust.works/api/v3/comment?id=14436827

But I can't use resolve_object:

https://sh.itjust.works/api/v3/resolve_object?q=https://sh.itjust.works/comment/14436827

Is this limitation intentional? It seems to me that they should either both return the object, or neither should return the object. When the comment is local this isn't a huge issue because the comment endpoint can be used instead of resolve_object, but it gets tricky when I'm trying to resolve an object from a non-local instance.

[Nothing4You]

with the changes in this PR comments will be resolvable by users as well, as the additional check within the resolve api no longer exists, and it just uses the underlying check when retrieving the comment.

only comment metadata is returned though, not comment contents, which is the same way it works on the comment api.

[dessalines]

@Nothing4You bump on this one: Nothing4You#5 , then we can approve and merge this.

[Nothing4You]

merged to unblock this PR, refactoring can then be done separately