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

tech(perf): revoir l'algo qui re-ordonne les champs pour de meilleurs perf #9919

Merged
merged 8 commits into from
Jan 22, 2024

Conversation

mfo
Copy link
Contributor

@mfo mfo commented Jan 17, 2024

No description provided.

@mfo mfo marked this pull request as draft January 17, 2024 14:21
@mfo mfo force-pushed the US/better-renumber branch 2 times, most recently from e88d29f to 91cd30b Compare January 18, 2024 12:37
@mfo mfo changed the title WIP : feat(add_type_de_champ): stop renumbering all procedure_revision_type_de_champ feat(add_type_de_champ): stop renumbering all procedure_revision_type_de_champ Jan 18, 2024
@mfo mfo marked this pull request as ready for review January 18, 2024 13:13
Copy link
Member

@LeSim LeSim left a comment

Choose a reason for hiding this comment

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

Bien joué, merci beaucoup pour cette réécriture 🚀 .

J'ai pas mal de remarques pour fignoler parce que cette partie est un poil touchy et du coup, j'ai l'impression que mettre du polish n'est pas du temps perdu.

app/models/procedure_revision.rb Show resolved Hide resolved
app/models/procedure_revision.rb Outdated Show resolved Hide resolved
app/models/procedure_revision.rb Outdated Show resolved Hide resolved
Comment on lines +270 to 273
def next_position_for(siblings:, after_coordinate: nil)
if siblings.to_a.empty? # first element of the list, starts at 0
0
elsif after_coordinate # middle of the list, between two items
after_coordinate.position + 1
else # last element of the list, end with last position + 1
siblings.to_a.last.position + 1
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def next_position_for(siblings:, after_coordinate: nil)
if siblings.to_a.empty? # first element of the list, starts at 0
0
elsif after_coordinate # middle of the list, between two items
after_coordinate.position + 1
else # last element of the list, end with last position + 1
siblings.to_a.last.position + 1
end
# either we are at the beginning of the list or after another item
def next_position_for(after_coordinate:)
if after_coordinate.nil? # first element of the list, starts at 0
0
else # after another item
after_coordinate.position + 1
end

Alors ce changement me paraît logique ... Néanmoins, j'ai peur que nos tests ne correspondent plus à la réalité :

  • je crois qu'avant un refacto récent de l'ui, le front pouvait appeler add_tdc sans préciser le after_coordinate pour ajouter un tdc en dernière position.
  • d'après mes essais, ce n'est plus le cas aujourd'hui
  • mais c'est toujours utilisé dans le setup des specs => ce changement risque d'en casser une palanquée.

Du coup, si t'es d'accord avec le change, je veux bien m'occuper du refacto des tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gros taff, et j'ai l'impression que c'est un gros tas de spaghetti autour de ça. Mais je comprends ton envie nettoyer cette dette. Etant donné que tu as validé, je compte merger et ouvrir une autre PR. (me reste a cleaner le renumber, qui est exactement sur ce cas d'usage)

app/models/procedure_revision.rb Outdated Show resolved Hide resolved
@mfo mfo force-pushed the US/better-renumber branch from 438888a to 2ac6925 Compare January 22, 2024 13:08
@mfo mfo force-pushed the US/better-renumber branch from 75f9070 to 0f194a5 Compare January 22, 2024 13:57
@mfo mfo enabled auto-merge January 22, 2024 13:57
@mfo mfo added this pull request to the merge queue Jan 22, 2024
Merged via the queue into demarches-simplifiees:main with commit 6e35758 Jan 22, 2024
15 checks passed
@mfo mfo deleted the US/better-renumber branch January 22, 2024 15:16
@mfo mfo self-assigned this Jan 25, 2024
@mfo mfo changed the title feat(add_type_de_champ): stop renumbering all procedure_revision_type_de_champ tech(perf): deplacer les champs est plus rapide Jan 25, 2024
@mfo mfo changed the title tech(perf): deplacer les champs est plus rapide tech(perf): revoir l'algo qui re-ordonne les champs pour de meilleurs perf Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: A Communiquer
Development

Successfully merging this pull request may close these issues.

2 participants