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

Jump to bottom

Utilisation des colonnes dans l'affichage et le filtrage #10875

Merged
merged 27 commits into from
Oct 16, 2024
Merged

Utilisation des colonnes dans l'affichage et le filtrage #10875

merged 27 commits into from
Oct 16, 2024

Conversation

LeSim
Copy link
Member

@LeSim LeSim commented Sep 27, 2024

No description provided.

@LeSim LeSim changed the title utilisation des colonnes dans l'affichage et le filtrage WIP utilisation des colonnes dans l'affichage et le filtrage Oct 4, 2024
@LeSim LeSim changed the base branch from add_id_to_column_second_part to main October 4, 2024 14:43
@LeSim LeSim marked this pull request as ready for review October 4, 2024 14:52
mfo
mfo reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@mfo mfo left a comment

Choose a reason for hiding this comment

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

c'est un grand oui 💍 1ere passe, jvais checkout pour clarifier mes incertitude

app/components/instructeurs/column_picker_component.rb Outdated Show resolved Hide resolved
app/types/sorted_column_type.rb Outdated Show resolved Hide resolved
app/types/sorted_column_type.rb Outdated Show resolved Hide resolved
app/controllers/instructeurs/procedures_controller.rb Show resolved Hide resolved
app/models/procedure_presentation.rb Outdated Show resolved Hide resolved
app/models/concerns/columns_concern.rb Outdated Show resolved Hide resolved
app/components/dossiers/notified_toggle_component/notified_toggle_component.html.haml Outdated Show resolved Hide resolved
app/models/column.rb Outdated Show resolved Hide resolved
app/models/concerns/columns_concern.rb Outdated Show resolved Hide resolved
app/types/filtered_column_type.rb Show resolved Hide resolved
@mfo mfo force-pushed the add_id_to_column_third_part branch 7 times, most recently from e44538d to 5ec3156 Compare October 7, 2024 09:36
@LeSim LeSim force-pushed the add_id_to_column_third_part branch 4 times, most recently from f409d2b to b808eb7 Compare October 9, 2024 13:17
@codecov Codecov
Copy link

codecov bot commented Oct 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.21053% with 36 lines in your changes missing coverage. Please review.

Project coverage is 84.41%. Comparing base (097074f) to head (5621edc).
Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
app/types/filtered_column_type.rb 0.00% 27 Missing ⚠️
app/services/dossier_filter_service.rb 93.05% 5 Missing ⚠️
app/models/export.rb 90.47% 2 Missing ⚠️
app/models/column.rb 88.88% 1 Missing ⚠️
app/models/procedure_presentation.rb 95.65% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #10875      +/-   ##
==========================================
- Coverage   84.51%   84.41%   -0.10%     
==========================================
  Files        1142     1145       +3     
  Lines       25243    25242       -1     
  Branches     4715     4703      -12     
==========================================
- Hits        21334    21309      -25     
- Misses       3909     3933      +24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LeSim LeSim force-pushed the add_id_to_column_third_part branch 8 times, most recently from 021c285 to 0667184 Compare October 15, 2024 13:31
@LeSim LeSim force-pushed the add_id_to_column_third_part branch from 0667184 to b2c108c Compare October 15, 2024 13:49
@LeSim LeSim changed the title WIP utilisation des colonnes dans l'affichage et le filtrage Utilisation des colonnes dans l'affichage et le filtrage Oct 15, 2024
@LeSim LeSim force-pushed the add_id_to_column_third_part branch from b2c108c to 87f87dc Compare October 15, 2024 13:59
LeSim and others added 21 commits October 15, 2024 16:09
@LeSim
remove now useless add_filters and remove_filters
4c5d7e2
@LeSim @mfo
use filtered_column to filter !
7e4ca07 
Co-authored-by: mfo <mfo@users.noreply.github.com>
@LeSim
use displayed_columns !
14fe11b
@LeSim
remove now unused code
6b5efbd
@LeSim @mfo
move validations concern to filtered_column
b2754cd 
Co-authored-by: mfo <mfo@users.noreply.github.com>
@LeSim
remove human_to_filter
0a54db6
@LeSim
default default_displayed_column
1448327
@mfo @LeSim
clean(deadcode): remove unused current_filters method
242ab78
@LeSim
extract dossier_state_column
30fcb75
@LeSim @mfo
tech(clean): remove Column classname: attribute
3677f3b 
Co-authored-by: mfo <mfo@users.noreply.github.com>
@LeSim @mfo
tech(clean): remove sva_columns(for_filters)
e41326d 
Co-authored-by: mfo <mfo@users.noreply.github.com>
@mfo @LeSim
tech(deadcode): remove unused constant on ProcedurePresentation
112d49c
@LeSim
reset procedure_presentation if a pb occurs with a column deserializa…
16e93a2 
…tion
@LeSim
extract filter and sort function to a filter_service
4d7715f
@LeSim
use the filter_service
d1530b4
@LeSim
remove useless Export.by_key procedure_presentation arg
603c2a1
@LeSim
add filtered and sorted columns to export
954d232
@LeSim
export cache_key based on column
b5ed8c9
@LeSim
remove useless snapshot
71bcbbc
@LeSim
remove procedure_presentation from export
f850924
@LeSim
remove unused filtered? method
5621edc
@LeSim LeSim force-pushed the add_id_to_column_third_part branch from 87f87dc to 5621edc Compare October 15, 2024 14:09
@mfo mfo closed this Oct 16, 2024
@mfo mfo reopened this Oct 16, 2024
mfo
mfo reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor

@mfo mfo left a comment

Choose a reason for hiding this comment

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

première passe

app/controllers/instructeurs/procedures_controller.rb Show resolved Hide resolved
app/controllers/recherche_controller.rb Show resolved Hide resolved
app/models/column.rb

def self.find(h_id)
Procedure.with_discarded.find(h_id[:procedure_id]).find_column(h_id:)
begin
Copy link
Contributor

@mfo mfo Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

j'imagine que rails fait sa magie pr cacher le truc, mais j'aimerais tester

Copy link
Member Author

Choose a reason for hiding this comment

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

cacher ?

Copy link
Contributor

@mfo mfo Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

qd on deserialize une Column, ca tappe la db. cf: https://github.com/demarches-simplifiees/demarches-simplifiees.fr/blob/main/app/types/column_type.rb#L20

bref, il y a une n+1. Ça arrive quand on a des filtres/displayed columns (que j'ai compris en cherchant), ça pourrait piquer avec des tableaux un peu plus personnalisés (bcp de displayed_columns).

Mais après recherche, je pense que c'est un cas trop a la marge pour s'en soucier vraiment/maintenant. Je pense qu'on peut accepter cette n+1 et voir si en prod ça devient génant. Qu'en penses-tu ?

cf: Capture d’écran 2024-10-16 à 2 27 17 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

j'ai comme toi en local un chargement en dixieme de ms, du coup, je te rejoins et je me dis qu'il vaudrait peut être mieux voir si c est un pb en prod. J'ai un peu peur du cache ...

@LeSim LeSim added this pull request to the merge queue Oct 16, 2024
Merged via the queue into main with commit 6862f6c Oct 16, 2024
36 checks passed
@LeSim LeSim deleted the add_id_to_column_third_part branch October 16, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfo mfo mfo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LeSim @mfo