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

Automation Rules: simplify ordering #10896

Merged
merged 7 commits into from
Nov 30, 2023
Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 8, 2023

I'm trying to re-use this for redirects, and found a couple of things that we can improve.

  • New items would be inserted first instead of at the end, if a user is adding a new item, they may want to have it priority over existing ones. Inserting the item at the end was mostly done because it was "easier" to implement.

  • Instead of explicitly using the manager to add a new rule, just override the save() method. This allows us to:

    • Insert a new rule with any priority
    • Change the priority of a rule by just saving it with the new priority.

    With this, we no longer need to override forms, serializers, etc, saving the model will take care of putting the rule in the right place.


📚 Documentation previews 📚

@stsewd stsewd marked this pull request as ready for review November 9, 2023 06:09
@stsewd stsewd requested review from a team as code owners November 9, 2023 06:09
@stsewd stsewd requested review from agjohnson and humitos November 9, 2023 06:09
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

The new approach looks better to me.

I think the _change_priority() is unnecessary complex and can be re-written in a lot more simple way. I proposed a way that's lot easier to understand and maintain.

readthedocs/builds/models.py Show resolved Hide resolved
readthedocs/builds/models.py Outdated Show resolved Hide resolved
readthedocs/builds/models.py Show resolved Hide resolved
@stsewd stsewd requested a review from humitos November 22, 2023 16:55
@stsewd stsewd merged commit 9cecf04 into main Nov 30, 2023
@stsewd stsewd deleted the simplify-automation-rules-ordering branch November 30, 2023 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants