fix: allow for custom states in status field

[ericswpark]

From #366 (comment)

Thanks to @cdevacc1 for the idea. I just decided to write up the PR as it only takes a couple of minutes. This should resolve the bug where custom states are not shown at all in the admin panel.

[auvipy]

And tests are already failing you can see that

[ericswpark]

Yup, forgot to create DB migrations after the change. (unless it was failing for a different reason)

[ericswpark]

@auvipy the tests should now pass with the added migration. Can I please request a review? Thanks!

[ericswpark]

@auvipy I'm not sure what breaking changes you're concerned about -- removing the line should allow for custom states while letting those who originally used the TASK_STATE_CHOICES for choices keep their data with no changes necessary. Please let me know if there's anything else you need or any other changes that should be made before the PR is merged or rejected. Thanks.

[ericswpark]

@auvipy may this please be reviewed before it becomes stale? I'd like to avoid having to rebase it over and over and this PR's been sitting in my list since last year. Thanks!

[auvipy]

@auvipy I'm not sure what breaking changes you're concerned about -- removing the line should allow for custom states while letting those who originally used the TASK_STATE_CHOICES for choices keep their data with no changes necessary. Please let me know if there's anything else you need or any other changes that should be made before the PR is merged or rejected. Thanks.

my only concern is if existing users are going to miss anything by the removal of the TASK_STATE_CHOICES

[ericswpark]

my only concern is if existing users are going to miss anything by the removal of the TASK_STATE_CHOICES

Please see previous comments. It should not affect existing users as it only limits what choices can go inside the field. Removing it expands it to allow for other values, not just the ones in TASK_STATE_CHOICES.

[auvipy]

@AllexVeldman can you review this as well, please?

[AllexVeldman]

LGTM

[auvipy]

can you please make the integration tests pass for this PR? with the change, may be it would be good idea to add/adjust additional tests

[AllexVeldman]

@ericswpark I think this will cover it:

def test_custom_state(self):
        tid = uuid()

        assert self.b.get_status(tid) == states.PENDING

        self.b.store_result(tid, state="Progress", result={"progress": 10})
        assert self.b.get_status(tid) == "Progress"
        assert self.b.get_result(tid) == {"progress": 10}

        self.b.mark_as_done(tid, 42)
        assert self.b.get_status(tid) == states.SUCCESS
        assert self.b.get_result(tid) == 42

Note that this does not fail on main since the backend never calls TaskResult.full_clean() so model validation seems to never be executed.

@auvipy This also means this change only changes the field in the Django admin pages from a dropdown to a text field, no functional changes here.

[auvipy]

Ok

[ericswpark]

@AllexVeldman thank you for the tests, I tried adding them to the tests directory under the tests for the models but am not sure that they belong there. I was hoping GitHub Actions/CI would kick in and verify but it seems like the workflow needs to be approved by a maintainer for them to run.

[ericswpark]

Ach, I think the migrations are failing because this PR is so old. At some point another migration file was pushed to main and then this PR was rebased on top without recreating migrations. Will recreate migrations and push again shortly.

[AllexVeldman]

I tried adding them to the tests directory under the tests for the models but am not sure that they belong there.

@ericswpark I should have mentioned :) this would go into /t/unit/backends/test_database.py since it tests setting a custom state, not specifically how the state is stored in the db model.

[ericswpark]

Sorry about that, I forgot to sync my main branch on my fork. The new migration should be based off of the last one (0014) created on main.