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.

Sign up for GitHub

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

5823 – Prevent an attempted relationship between items across teams #2149

Closed
wants to merge 8 commits into from
Closed

5823 – Prevent an attempted relationship between items across teams #2149

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
46534fb
test first draft
vasconsaurus Dec 4, 2024
4b64706
refactor to fix invalid team errors
DGaffney Dec 5, 2024
8c4a573
add in fix for explainer as well
DGaffney Dec 5, 2024
633bd34
raise "Related items must exist in the same workspace"
vasconsaurus Dec 5, 2024
666689b
remove new tests
vasconsaurus Dec 5, 2024
eb072c1
Merge branch '5676-alegre-bot-must-filter-invalid-results' of github.…
vasconsaurus Dec 5, 2024
dba3e81
codeclimate issues
vasconsaurus Dec 10, 2024
7f4a1a4
add Devin's update
vasconsaurus Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions app/lib/smooch_nlu.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ def self.alegre_matches_from_message(message, language, context, alegre_result_k
end
begin
self.increment_global_counter
team_slug = Team.find(Bot::Smooch.config['team_id']).slug
team_id = Bot::Smooch.config['team_id']
team_slug = Team.find(team_id).slug
# FIXME: In the future we could consider matches across all languages when options is nil
# FIXME: No need to call Alegre if it's an exact match to one of the keywords
# FIXME: No need to call Alegre if message has no word characters
Expand All @@ -85,7 +86,9 @@ def self.alegre_matches_from_message(message, language, context, alegre_result_k
language: language,
}.merge(context)
}
response = Bot::Alegre.query_sync_with_params(params, "text")
response = Bot::Alegre.query_sync_with_params(params, "text")["result"].collect{|result|
result["context"] = Bot::Alegre.isolate_relevant_context(team_id, result)
}

# One approach would be to take the option that has the most matches
# Unfortunately this approach is influenced by the number of keywords per option
Expand All @@ -96,7 +99,7 @@ def self.alegre_matches_from_message(message, language, context, alegre_result_k
# ranked_options = option_counts.group_by(&:itself).transform_values(&:count).sort_by{|_k,v| v}.reverse()

# Second approach is to sort the results from best to worst
sorted_options = response['result'].to_a.sort_by{ |result| result['score'] }.reverse
sorted_options = response.to_a.sort_by{ |result| result['score'] }.reverse
ranked_options = sorted_options.map{ |o| { 'key' => o.dig('context', alegre_result_key), 'score' => o['score'] } }
matches = ranked_options

Expand Down
32 changes: 18 additions & 14 deletions app/models/concerns/alegre_v2.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,8 @@ def get_per_model_threshold(project_media, threshold)
end
end

def isolate_relevant_context(project_media, result)
(result["contexts"]||result["context"]).select{|x| ([x["team_id"]].flatten & [project_media.team_id].flatten).count > 0 && !x["temporary_media"]}.first
def isolate_relevant_context(team_id, result)
(result["contexts"]||result["context"]).select{|x| ([x["team_id"]].flatten & [team_id].flatten).count > 0 && !x["temporary_media"]}.first
Copy link
Contributor

Choose a reason for hiding this comment

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

probably needs to be [(result["contexts"]||result["context"])].flatten.select.... instead here

end

def get_target_field(project_media, field)
Expand All @@ -350,18 +350,22 @@ def get_target_field(project_media, field)
def parse_similarity_results(project_media, field, results, relationship_type)
results ||= []
Hash[results.collect{|result|
result["context"] = isolate_relevant_context(project_media, result)
[
result["context"] && result["context"]["project_media_id"],
{
score: result["score"],
context: result["context"],
model: result["model"],
source_field: get_target_field(project_media, field),
target_field: get_target_field(project_media, result["field"] || result["context"]["field"]),
relationship_type: relationship_type
}
]
result["context"] = isolate_relevant_context(project_media.team_id, result)
if result["context"]
[
result["context"] && result["context"]["project_media_id"],
{
score: result["score"],
context: result["context"],
model: result["model"],
source_field: get_target_field(project_media, field),
target_field: get_target_field(project_media, result["field"] || result["context"]["field"]),
relationship_type: relationship_type
}
]
else
raise ActiveRecord::RecordInvalid.new, "Related items must exist in the same workspace"
end
}.reject{ |k,_| k == project_media.id }]
end

Expand Down
6 changes: 4 additions & 2 deletions app/models/explainer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,10 @@ def self.search_by_similarity(text, language, team_id)
language: language
}
}
response = Bot::Alegre.query_sync_with_params(params, "text")
results = response['result'].to_a.sort_by{ |result| result['_score'] }
response = Bot::Alegre.query_sync_with_params(params, "text")["result"].collect{|result|
result["context"] = Bot::Alegre.isolate_relevant_context(team_id, result)
Copy link
Contributor

Choose a reason for hiding this comment

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

For reasons I don't understand, Explainers appear to be indexed in Alegre with the team slug rather than the team_id. As a result, this won't work.

I think the correction action is to index explainers with team_id, but whatever the solution we need to work that out first.

}
results = response.to_a.sort_by{ |result| result['_score'] }
explainer_ids = results.collect{ |result| result.dig('context', 'explainer_id').to_i }.uniq.first(3)
explainer_ids.empty? ? Explainer.none : Explainer.where(team_id: team_id, id: explainer_ids)
end
Expand Down
8 changes: 4 additions & 4 deletions test/models/bot/alegre_v2_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,22 +280,22 @@ def teardown

test "should isolate relevant_context for text" do
pm1 = create_project_media team: @team, quote: 'This is a long text that creates a text-based item'
assert_equal Bot::Alegre.isolate_relevant_context(pm1, {"context"=>[{"team_id"=>pm1.team_id}]}), {"team_id"=>pm1.team_id}
assert_equal Bot::Alegre.isolate_relevant_context(pm1.team_id, {"context"=>[{"team_id"=>pm1.team_id}]}), {"team_id"=>pm1.team_id}
end

test "should isolate relevant_context for audio" do
pm1 = create_project_media team: @team, media: create_uploaded_audio
assert_equal Bot::Alegre.isolate_relevant_context(pm1, {"context"=>[{"team_id"=>pm1.team_id}]}), {"team_id"=>pm1.team_id}
assert_equal Bot::Alegre.isolate_relevant_context(pm1.team_id, {"context"=>[{"team_id"=>pm1.team_id}]}), {"team_id"=>pm1.team_id}
end

test "should isolate relevant_context for image" do
pm1 = create_project_media team: @team, media: create_uploaded_image
assert_equal Bot::Alegre.isolate_relevant_context(pm1, {"context"=>[{"team_id"=>pm1.team_id}]}), {"team_id"=>pm1.team_id}
assert_equal Bot::Alegre.isolate_relevant_context(pm1.team_id, {"context"=>[{"team_id"=>pm1.team_id}]}), {"team_id"=>pm1.team_id}
end

test "should isolate relevant_context for video" do
pm1 = create_project_media team: @team, media: create_uploaded_video
assert_equal Bot::Alegre.isolate_relevant_context(pm1, {"context"=>[{"team_id"=>pm1.team_id}]}), {"team_id"=>pm1.team_id}
assert_equal Bot::Alegre.isolate_relevant_context(pm1.team_id, {"context"=>[{"team_id"=>pm1.team_id}]}), {"team_id"=>pm1.team_id}
end

test "should return field or type on get_target_field for text" do
Expand Down
Loading