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

Document re-evaluation of routing rules on signal updates #1122

Conversation

tcoenen
Copy link
Contributor

@tcoenen tcoenen commented Nov 24, 2022

Description

A part of the discussions around PR#1120 was the need to document the behavior of Routing expressions on updates to the underlying nuisance complaint (Dutch: melding). This PR adds that documentation.

Checklist

  • Keep the PR, and the amount of commits to a minimum
  • The commit messages are meaningful and descriptive
  • Check that the branch is based on main and is up to date with main
  • Check that the PR targets main
  • There are no merge conflicts and no conflicting Django migrations

@tcoenen
Copy link
Contributor Author

tcoenen commented Nov 24, 2022

@bartjkdp What is the name of the feature flag?

@bartjkdp
Copy link
Contributor

bartjkdp commented Nov 24, 2022

@tcoenen if you want, you can propose the name of the feature flag. I will implement it accordingly in #1120.

@tcoenen
Copy link
Contributor Author

tcoenen commented Nov 24, 2022

@bartjkdp I don't have a particular name in mind. You could use DSL_RUN_ROUTING_EXPRESSIONS_ON_UPDATES if you have no better name for it yourself.

@bartjkdp
Copy link
Contributor

@tcoenen I like this name, I will use it in my PR as well.

@tcoenen
Copy link
Contributor Author

tcoenen commented Nov 26, 2022

I updated the document with the correct feature flag name. I think this document is now complete and I will change the PR status to ready for review.

@tcoenen tcoenen marked this pull request as ready for review November 26, 2022 09:35
Copy link
Member

@vanbuiten vanbuiten left a comment

Choose a reason for hiding this comment

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

Can you add these changes to PR #1120 instead of main. That way we can merge the whole functionality as one. OR we should merge #1120 and after that #1122.

Copy link
Member

@vanbuiten vanbuiten left a comment

Choose a reason for hiding this comment

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

Approved

@vanbuiten vanbuiten merged commit 6ae69ad into Amsterdam:main Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants