-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Update Work Package subject using a Type defined blueprint #17516
Conversation
a34c017
to
fe14ed9
Compare
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.
It's looking pretty good. I can take another look after it is moved from draft.
return true unless work_package.type&.replacement_patterns_defined? | ||
|
||
if work_package.type.patterns.all_enabled[:subject] | ||
work_package.subject = "Templated by #{work_package.type.name}" |
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.
Not sure I follow. Is this a temporary subject? Does this get persisted?
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.
Yes. If there's no subject the contract will fail as it is a required field, so I fill in with something for now so that we can overwrite it later on.
It will get persisted as I might need "persisted" info (like created_at
) for the pattern, but all is done within the context of the same request, so the user won't (hopefully) see it.
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.
Fair enough. Makes sense to me.
... I thought we might want to somehow tag these so that we can query any lingering/stuck cases later. But honestly Templated by
is probably good enough for that.
contract_class:) | ||
.call(attributes) | ||
def set_templated_subject(work_package) | ||
return true unless work_package.type&.replacement_patterns_defined? |
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.
We're doing this a few times. Does it make sense to add a helper in the work_package for syntax sugar?
work_package.type&.replacement_patterns_defined?
work_package.replacement_patterns_defined?
... probably no 😅
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.
I wondered the same thing. Ended up not doing it... for now. :P
be55424
to
0cda98d
Compare
14306e0
to
426dbea
Compare
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.
I think it looks good. It's still marked as draft, but looks right to me.
Left a few less important comments
|
||
module Types | ||
class PatternResolver | ||
TOKEN_REGEX = /{{[0-9A-Za-z_]+}}/ |
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.
Is this the final regex? Don't we need to support .
or any other symbols?
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.
I am thinking we might need to concat field names and context, like {{parent.assignee}}
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.
Yeah, I went with just prefix_
as it proved easier to read and less of hassle to parse.
I frigging could swear I clicked the "Ready for Review" button. 🤦🏼 |
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.
🚢
Related WP: OP#60003
Wat?
This PR aims to make possible the definition/replacement of a
WorkPackage#subject
based on blueprint defined on itsType
.How?
There are a lot of moving parts here. The
PatternCollection
,Pattern
and their ActiveRecord value types where already in place.Now comes the
PatternResolver
that is capable of taking the blueprint from aPattern
parsing it and resolving it into a string for replacement.To update the Work Packages, changes where necessary on the
SetAttributeService
,CreateService
andUpdateService
.Details
PatternResolver
resolves the tokens (basically fragments on double curly braces, i.e.{{token}}
) usingTokenPropertyMapper
.The
TokenPropertyMapper
is the source of allproperty -> token
information, including which tokens are available. When called tries to find the key on its predefined attributes and if it is not found, it tries to call the method on the passed context and get its value. It also implements the I18n layer for the names of all the properties.Things get more complicated when we get to custom fields, as those are dynamically added to work packages and can vary from installation to installation.
But all custom field assigned values can be accessed via the
custom_field_ID
method, so we first check the context:{{custom_field_123}}
is considered part of the Work Package{{parent_custom_field_123}}
will try to getwork_package.parent.custom_field_123
{{project_custom_field_123}}
will try to get the project attribute 123 in the same way above.The
SetAttributesService
needed to be updated so that it overrides whatever subject was there in the first place with a message saying that it will be updated later. This is important in case something fails later on.Update and Create services do their respective updates very late in the process, to make sure all the changes that might impact the subject are already assigned.
On Create specifically we save first, just in case we need some auto-generated field like
created_at
.