-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
[#7293] Update Notable#all_notes #8408
Conversation
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.
haven't looked in detail but looks good and be good to have this working
@@ -4,12 +4,20 @@ | |||
describe '#all_notes' do | |||
subject { record.all_notes } | |||
|
|||
before { record.tag_string = 'foo' } | |||
before { record.tag_string = 'foo:1' } |
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.
might be worth making this an additional spec so that we're testing both foo
and foo:1
?
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.
Yep good shout, I have added this in a fixup.
Before this PR we had:
- PublicBody tagged with "foo:bar" would be associated with a note with tagged with "foo:bar" but not "foo"
- PublicBody tagged with "foo" would be associated with a note with tagged with "foo" but not "foo:bar"
After this PR:
- PublicBody tagged with "foo:bar" would be associated with a note with tagged with "foo:bar" or "foo"
- PublicBody tagged with "foo" would be associated with a note with tagged with "foo" but not "foo:bar"
What do you think about that last case... should a PublicBody tagged with "foo" be associated with a note with tagged with "foo:bar".
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.
After this PR:
- PublicBody tagged with "foo:bar" would be associated with a note with tagged with "foo:bar" or "foo"
- PublicBody tagged with "foo" would be associated with a note with tagged with "foo" but not "foo:bar"
This sounds like the correct behaviour to me.
Update the concern so that records tagged with `name:value` will be associated with notes tagged as `name`. Fixes #7293
6c53468
to
8496335
Compare
Relevant issue(s)
Fixes #7293
What does this do?
Update Notable#all_notes
Why was this needed?
Update the concern so that records tagged with
name:value
will be associated with notes tagged asname
.