-
Notifications
You must be signed in to change notification settings - Fork 7
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
Include link to wiki in the second slo action #52
Conversation
|
edf9a77
to
2edc469
Compare
Use reminder_text and extend it with the expected comment to describe the action of the second reminder. The reason of the rewording is to make obvious that both reminders belong together. `msg` variable is moved outside of the individual functions which perform the reminder to avoid duplication. https://progress.opensuse.org/issues/119176#note-23 Signed-off-by: ybonatakis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should the message now include the explanation and wiki link? Can you show an update on a example ticket or something?
feels like you overlooked my question in #52 (review) . can you answer that please? |
I havent run it but the changes should just make the comment exactly as we agree |
slo_priorities[priority_current]["next_priority"]["name"], | ||
poo_id)) | ||
url = "{}/{}.json".format(data["web"], poo_id) | ||
msg = " ".join(update_slo_text.format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I execute this I think it's not what we want :D
>>> update_slo_text = "The ticket will be set to the next lower priority **{priority}**."
>>> msg = " ".join(update_slo_text.format(priority=23))
>>> msg
'T h e t i c k e t w i l l b e s e t t o t h e n e x t l o w e r p r i o r i t y * * 2 3 * * .'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And @b10n1k please see if you can add test coverage - since I mistakenly thought this was covered, which is why I thought it must still work
https://progress.opensuse.org/issues/119176#note-23