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

Permet d'ajouter des filtres aux institutions #3462

Merged
merged 12 commits into from
Jun 5, 2024

Conversation

LucienMLD
Copy link
Collaborator

closes #3461

@LucienMLD LucienMLD marked this pull request as draft May 27, 2024 13:07
@LucienMLD LucienMLD marked this pull request as ready for review May 27, 2024 14:43
@LucienMLD LucienMLD force-pushed the lucien/filtre-institutions branch from 1cde1ea to be6dd42 Compare May 27, 2024 15:27
@LucienMLD LucienMLD changed the title WIP Permet d'ajouter des filtres aux institutions Permet d'ajouter des filtres aux institutions May 28, 2024
@LucienMLD LucienMLD force-pushed the lucien/filtre-institutions branch from 9d5cf35 to a7b8845 Compare May 28, 2024 09:31
@LucienMLD LucienMLD requested a review from clairezed May 28, 2024 09:56
@clairezed clairezed force-pushed the lucien/filtre-institutions branch from a7b8845 to 3dcc8ea Compare May 28, 2024 14:15
Copy link
Collaborator

@clairezed clairezed left a comment

Choose a reason for hiding this comment

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

Hop, j'ai relevé de possibles subtilités, à discuter éventuellement ensemble

app/models/match_filter.rb Outdated Show resolved Hide resolved
app/models/expert_subject.rb Outdated Show resolved Hide resolved
@LucienMLD LucienMLD force-pushed the lucien/filtre-institutions branch 2 times, most recently from fc17f25 to 87d59b7 Compare May 30, 2024 15:50
Copy link
Collaborator

@clairezed clairezed left a comment

Choose a reason for hiding this comment

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

des commentaires peu importants en attendant la suite des tests

app/models/match_filter.rb Outdated Show resolved Hide resolved
app/models/match_filter.rb Show resolved Hide resolved
@LucienMLD LucienMLD force-pushed the lucien/filtre-institutions branch from 87d59b7 to f8d7af1 Compare June 3, 2024 14:13
@LucienMLD
Copy link
Collaborator Author

@clairezed c'est bon pour les tests je pense, j'ai fait un filtre normal et un filtre avec sujet

@clairezed clairezed force-pushed the lucien/filtre-institutions branch from f8d7af1 to 5b21d37 Compare June 3, 2024 14:56
Copy link
Collaborator

@clairezed clairezed left a comment

Choose a reason for hiding this comment

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

J'ai juste testé la création de filtres en admin, mais pas une mise en action concrète avec différents filtres et tout (trop complexe de créer un vrai jeu de données).

Je fais confiance aux tests pour les cas concrets avec données !

Aussi, j'ai tenté un truc pour choper les match_filters d'un expert_subject, cf le dernier commit.

Ca permet de passer de :

ExpertSubject Load (2.1ms)  SELECT "experts_subjects".* FROM "experts_subjects" ORDER BY "experts_subjects"."id" DESC LIMIT $1  [["LIMIT", 1]]                
Expert Load (0.7ms)  SELECT "experts".* FROM "experts" WHERE "experts"."id" = $1 LIMIT $2  [["id", 22938], ["LIMIT", 1]]                                      
Antenne Load (1.1ms)  SELECT "antennes".* FROM "antennes" WHERE "antennes"."id" = $1 LIMIT $2  [["id", 952], ["LIMIT", 1]]                                    
MatchFilter Pluck (0.7ms)  SELECT "match_filters"."id" FROM "match_filters" WHERE "match_filters"."filtrable_element_id" = $1 AND "match_filters"."filtrable_element_type" = $2  [["filtrable_element_id", 952], ["filtrable_element_type", "Antenne"]]                                                                       
Institution Load (2.2ms)  SELECT "institutions".* FROM "institutions" INNER JOIN "antennes" ON "institutions"."id" = "antennes"."institution_id" WHERE "antennes"."id" = $1 LIMIT $2  [["id", 952], ["LIMIT", 1]]                             
MatchFilter Pluck (0.4ms)  SELECT "match_filters"."id" FROM "match_filters" WHERE "match_filters"."filtrable_element_id" = $1 AND "match_filters"."filtrable_element_type" = $2  [["filtrable_element_id", 15], ["filtrable_element_type", "Institution"]]

à

ExpertSubject Load (0.4ms)  SELECT "experts_subjects".* FROM "experts_subjects" ORDER BY "experts_subjects"."id" DESC LIMIT $1  [["LIMIT", 1]]
MatchFilter Load (1.0ms)  SELECT "match_filters".* FROM "match_filters" INNER JOIN "antennes" ON "match_filters"."filtrable_element_id" = "antennes"."id" INNER JOIN "experts" ON "antennes"."id" = "experts"."antenne_id" WHERE "experts"."id" = $1 AND "match_filters"."filtrable_element_type" = $2  [["id", 22938], ["filtrable_element_type", "Antenne"]]
MatchFilter Load (1.3ms)  SELECT "match_filters".* FROM "match_filters" INNER JOIN "institutions" ON "match_filters"."filtrable_element_id" = "institutions"."id" INNER JOIN "antennes" ON "institutions"."id" = "antennes"."institution_id" INNER JOIN "experts" ON "antennes"."id" = "experts"."antenne_id" WHERE "experts"."id" = $1 AND "match_filters"."filtrable_element_type" = $2  [["id", 22938], ["filtrable_element_type", "Institution"]]

config/locales/models.fr.yml Outdated Show resolved Hide resolved
@@ -2,4 +2,12 @@

FactoryBot.define do
factory :match_filter

trait :for_antenne do
Copy link
Collaborator

Choose a reason for hiding this comment

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

ça marche aussi comme ça pour les polymorphics ? Dans la doc, je vois autre chose (https://thoughtbot.github.io/factory_bot/cookbook/polymorphic-associations.html)

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, c'est grâce aux getters et setters que tu as mis en place, je présume

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

J'ai trouvé ça plus pratique d'avoir des setters

spec/models/match_filter_spec.rb Outdated Show resolved Hide resolved
let(:diagnosis) { create :diagnosis, company: company }
let(:need) { create :need, diagnosis: diagnosis }
let!(:es_01) { create :expert_subject }

context 'min_years_of_existence' do
let(:match_filter_01) { create :match_filter, min_years_of_existence: 3 }
describe 'min_years_of_existence' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

je les trouve pas lisibles ces tests, car à chaque fois les sample de dates de création de l'entreprise changent, ce qui complique la comparaison des effets de l'application des différents filtres. Y'a moyen de garder les mêmes dates pour chaque context ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Je vais essayer oui, c'est pas simple je me suis pas mal pris la tête là-dessus pour avoir des trucs cohérents.

add_reference :match_filters, :filtrable_element, polymorphic: true, index: true

MatchFilter.find_each do |match_filter|
match_filter.update(filtrable_element: match_filter.antenne)
Copy link
Collaborator

Choose a reason for hiding this comment

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

comme tu as créé un getter qui s'appelle "antenne" pour les match_filters, la migration ne fonctionne pas écrite comme ça (tous les filtrables sont à nil). Peut-être en allant plutôt chercher le champs antenne_id ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

j'ai pas retesté depuis le passage en polymorphisme, je regarde ça

@LucienMLD LucienMLD force-pushed the lucien/filtre-institutions branch from 94a84f8 to 63b8444 Compare June 4, 2024 14:19
@LucienMLD
Copy link
Collaborator Author

@clairezed ok pour deuxième revue

@LucienMLD LucienMLD force-pushed the lucien/filtre-institutions branch 2 times, most recently from 16c471b to 3cdc3ea Compare June 5, 2024 13:12
@@ -0,0 +1,22 @@
class UpdateMatchFilterRelationToPolymorphic < ActiveRecord::Migration[7.0]
def up
add_reference :match_filters, :filtrable_element, polymorphic: true, index: true, null: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

je vois toujours le null: false ici, pourtant @LucienMLD

@LucienMLD LucienMLD force-pushed the lucien/filtre-institutions branch from 3cdc3ea to d8d8263 Compare June 5, 2024 13:37
@LucienMLD LucienMLD force-pushed the lucien/filtre-institutions branch from 0e0d8b7 to fcd0c10 Compare June 5, 2024 13:51
@LucienMLD LucienMLD merged commit c7ef995 into main Jun 5, 2024
3 checks passed
@LucienMLD LucienMLD deleted the lucien/filtre-institutions branch June 5, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FILTRES AUX INSTITUTIONS :
2 participants