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

Add move method to automation rule #5998

Merged
merged 19 commits into from Sep 4, 2019

Conversation

@stsewd
Copy link
Member

commented Jul 26, 2019

This is needed to re-organize rules
without explicit changing all priorities
or changing one by one

I just need to add tests for negative steps, and add a comment why just doing an .update(F('priority') + 1) doesn't work

Add move method to automation rule
This is needed to re-organize rules
without explicit changing all priorities
or changing one by one
@stsewd

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

Oops, I use the manager introduced in #5995, that PR should be merged first

@humitos

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

What does it mean to "move an automation rule N steps"?

@stsewd

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

@humitos if you have this rules

  1. One
  2. Two
  3. Three

The automation rules are run in that order (according to their priority). User can rearrange the rules to change the order they are run. Like

  1. Three
  2. One
  3. Two

Users shouldn't need to update each priority version one by one. Think of it like a drag and drop thing

@humitos
Copy link
Member

left a comment

I feel this implementation is too complex and does not manage moving one step up (like steps=-1, at least in the tests).

I think we can implement this in a simpler way:

  1. query all the Automation Rules sorted by priority
  2. get the index of the one that we want to move from that list
  3. move that Automation Rule where it should be in the list (steps)
  4. set the index of each Automation Rule in the list as priority

So, we will end up always with 0, 1, 2, 3 as priority if we have 4 rules, instead of dealing with weird priority numbers.

@stsewd

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

I feel this implementation is too complex and does not manage moving one step up (like steps=-1, at least in the tests).

It does, tests are still missing. It's on the description.

move that Automation Rule where it should be in the list (steps) set the index of each Automation Rule in the list as priority

We can't save the values like that, it breaks the constraint of unique (project, priority)

So, we will end up always with 0, 1, 2, 3 as priority if we have 4 rules, instead of dealing with weird priority numbers.

Current implementation always keeps the range, it doesn't add weird numbers

@humitos

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

We can't save the values like that, it breaks the constraint of unique (project, priority)

Why it will break that constraint if we will have priorities like 0, 1, 2, 3, 4 tied to one project?

Current implementation always keeps the range, it doesn't add weird numbers

This is the weird number that I was reffering to: https://github.com/readthedocs/readthedocs.org/pull/5998/files#diff-763de362f34a86b264903b372e7dd27fR1012

@stsewd

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

This is the weird number that I was reffering to

That's is temporary, to avoid the unique constraint

Why it will break that constraint if we will have priorities like 0, 1, 2, 3, 4 tied to one project?

In the final result not, but in the intermediate steps, when you try to save or update an object, other object will have the same priority. That's why I can't use just one update in the current code and need to iterate over each object.

@stsewd stsewd removed the Needed: tests label Jul 30, 2019

@stsewd stsewd requested a review from readthedocs/core Jul 30, 2019

@stsewd

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

This is ready for review. This is on top of #5995 (that should be merged first)

readthedocs/builds/managers.py Outdated Show resolved Hide resolved

@stsewd stsewd requested a review from readthedocs/core Aug 14, 2019

@stsewd

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

I also overrode the delete method to keep the priority numbers the same after delete

# avoid hitting the unique constraint (project, priority).
for rule in rules:
rule.priority = expression
rule.save()

This comment has been minimized.

Copy link
@ericholscher

ericholscher Aug 26, 2019

Member

What happens if we hit an exception here? The rule will be stuck with a broken priority. We likely need this to happen in some kind of DB transaction to avoid bad states here.

This feels really complicated and brittle. I don't have a better suggestion currently, but I'm wondering if this concept of priority is something we need to enforce so strongly.

This comment has been minimized.

Copy link
@stsewd

stsewd Aug 26, 2019

Author Member

We have transactions per request

ATOMIC_REQUESTS = True

So, we need to have a unique priority in all rules of a project, that way they get sorted how the user wants.

We can remove the unique constraint as Manuel suggested, and maybe check for inconsistencies on save?

This comment has been minimized.

Copy link
@ericholscher

ericholscher Aug 27, 2019

Member

Yea, I guess we can keep this for now, but I'm guessing it will cause issues at some point :)

readthedocs/builds/models.py Outdated Show resolved Hide resolved
stsewd and others added 4 commits Aug 26, 2019
update docstring
Co-Authored-By: Eric Holscher <25510+ericholscher@users.noreply.github.com>

@stsewd stsewd requested a review from humitos Aug 27, 2019

@humitos
Copy link
Member

left a comment

I'm not happy with the implementation, but feel free to merge if you want. @ericholscher already approved it 😄. Although, ...

I think that having the constrain at the db level for this just makes our lives harder and does not gives us too much benefit, since the order/priority will always set programmatically (not manual).

I feel it's easier to maintain all this code (with a simple implementation) if we just remove the constrain from the db.

@stsewd stsewd merged commit 89a58a0 into readthedocs:master Sep 4, 2019

2 checks passed

continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd:add-move-method branch Sep 4, 2019

@ericholscher

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

I like having the DB restriction though. I don't trust code not to mess it up, and then we'd end up with weird bugs. This will lead to different weird bugs, but at least these weird bugs will be understandable :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.