Bugfix #69

felginep · 2021-08-25T06:12:28Z

Fix #58

Add correct warning messages when spreadsheet is empty

WARN-- : Your spreadsheet is empty. Add content and retry.

Fix #59 #67

Add warning messages when spreadsheet is corrupted.

With the following spreadsheet:

,key,en,
,aaa.bbb,first,
,aaa.bbb.ccc,second,
,bbb.ccc.ddd,first,
,bbb.ccc,second,
,zzz,first

The warnings emitted are:

WARN -- : Corrupted input. Trying to insert a value for key 'aaa.bbb.ccc' but a value already exists for 'aaa.bbb'. Skipping.
WARN -- : Corrupted input. Trying to insert a value for key 'bbb.ccc' but values already exist for keys 'bbb.ccc.*'. Previous values will be lost.

Fix #61

There was a performance issue that was slowing down the export. The issue was that we looped through all the existing_keys (the keys of the csv we already treated) for each key of the csv. This was useless and inefficient.
The goal was to avoid issues when there are duplicates or plural / singular mismatch.

The solution is to use a hash table to store the treated keys along with their label. That way, if a label already exist in the hash table, that means this is not the first time we see the key.

Some warnings have been added to notify the user that the spreadsheet is bad. With the following spreadsheet:

,key,fr,comment fr,en,comment en
#Generic actions,,,,,
,duplicate,Duplicate,,Duplicate,
,duplicate,Erreur,,Error,
#Plurals,,,,,
,plural_before_singular##{one},Plural before Singular,,Plural before Singular,
,plural_before_singular##{one},Plural before Singular,,Plural before Singular,
,plural_before_singular##{other},Plural before Singular,,Plural before Singular,
,plural_before_singular,Erreur,,Error,,
,singular_before_plural,Singular before Plural,,Singular before Plural,,
,singular_before_plural##{one},Erreur,,Error,,

The warnings emitted are:

WARN -- : Some values already exist for key 'duplicate'. Remove duplicates.
WARN -- : Some values already exist for key 'plural_before_singular'. Remove duplicates.
WARN -- : A plural value already exist for key 'plural_before_singular'. Remove duplicates.
WARN -- : A singular value already exist for key 'singular_before_plural'. Remove duplicates.

When downloading an empty spreadsheet, the content type of the downloaded file is "inode/x-empty" When shared configuration is misconfigured, the content type of the downloaded file is "text/html"

For each key, we looped through all the keys already read. This was very unefficient. Now we use a hash table to store the previous keys. Results: - 5 minutes export before optimization - less than 5 seconds after optimization What's more, we added some warnings along the way when we see duplicates, or singular / plural mismatch.

jvigne · 2021-08-25T08:30:52Z

lib/ad_localize/interactors/export_g_spreadsheet.rb

        else
-          LOGGER.error("invalid export request. check the spreadsheet share configuration")
+          if export_request.has_empty_files?


Tu peux utiliser un elsif ici, ça devrait donner le même résultat

Bien vu, je corrige

jvigne · 2021-08-25T08:39:34Z

lib/ad_localize/mappers/csv_path_to_wording.rb

+            existing_plural_key = existing_key.label == current_key.label && existing_key.plural? && current_key.singular?
+            existing_singular_key = existing_key.label == current_key.label && existing_key.singular? && current_key.plural?


Tu peux ajouter 2 méthodes dans key ? Une same_singular_as? et l'autre same_plural_as? Ça va compléter l'interface de la classe, qui a déjà same_as? et ça simplifiera le code ici.

Hum tu vois cette implémentation pour tes deux méthodes :

def same_singular_as?(key:) label == key.label && key.singular? end def same_plural_as?(key:) label == key.label && key.plural? end

Parce que vu le naming, j'aurais plutôt tendance à faire cette implémentation :

def same_singular_as?(key:) label == key.label && singular? && key.singular? end def same_plural_as?(key:) label == key.label && plural? && key.plural? end

Or du coup ca ne correspond plus à la condition de mon code, qui vérifie que bien que les labels soient égaux, les singuliers / pluriels ne correspondent pas.

Ah bon point 🤔, je pensais à une implémentation équilivalente au code de existing_plural_key et existing_singular_key mais c'est vrai que same_**_as? évoque une égalité totale. Le naming ne convient pas.

Tu en as un autre en tête ? J'ai pensé à same_label_as_singular?|same_label_as_plural? ou singular_with_same_label_as_plural?|plural_with_same_label_as_singular? ou singular_with_same_plural_as?| plural_with_same_singular_as?

Hum pas évident le naming ici. Je vais réfléchir. D'autant qu'au final ca me fait bizarre d'avoir cette méthode sur Key directement. Peut être qu'il faudrait juste en faire une méthode privée de la classe.

Aussi, ça m'irait

jvigne · 2021-08-25T08:43:11Z

lib/ad_localize/mappers/locale_wording_to_hash.rb

@@ -10,6 +10,14 @@ def map(locale_wording:)
                hash[translation.key.label] = {} unless hash.key? translation.key.label
                hash[translation.key.label][translation.key.plural_key] = translation.value
              else
+                unless hash.is_a?(Hash)


comment hash pourrait ne pas être un hash ? Ça voudrait dire que le code fait une mutation non voulue puisque hash est initialisé comme un hash avec le each_with_object({})

Ce ne serait pas hash[inner_key.to_s] que tu veux vérifier ?

hash peut être une string. Par exemple si tu veux inserer les deux paires suivantes dans cet ordre:

aaa.bbb => first aaa.bbb.ccc => second

Une fois que tu as inséré la première clé et que tu passes dans la seconde, tu as

hash === { "aaa" => { "bbb" => "first } }

Donc quand tu arrives à inner_key === "ccc", ton hash vaut en fait "first" et pas {}.

Ah oui je vois, c'est la L25 qui fait la mutation de hash hash = hash[inner_key]. C'est bon pour moi

jvigne

Merci pour cette PR, dans l'ensemble c'est bon. Je t'ai suggéré quelques améliorations liées au coding style et je t'ai posé une question sur LocaleWordingToHash

felginep · 2021-08-25T09:57:26Z

Pour le problème de perf, je vois qu'on a un code similaire ici. Je vais voir pour le modifier également.

jvigne · 2021-08-25T11:07:25Z

Super, cette modif devrait avoir beaucoup d'impact pour la majorité des utilisateurs car il est impliqué dans l'export depuis une spreadsheet

jvigne · 2021-08-26T09:39:30Z

lib/ad_localize/entities/key.rb

+      def ==(o)
+        o.class == self.class &&
+        o.raw_label == raw_label
+      end
+


excellent, c'est super d'y avoir pensé

jvigne · 2021-08-26T09:40:23Z

lib/ad_localize/mappers/csv_path_to_wording.rb

-          end
-          translations.concat(new_translations)
+          current_key = row_translations.first.key
+          next if validator.has_warnings?(current_key)


C'est beau 😄

felginep added 3 commits August 24, 2021 09:43

Fix #58 : Fix error warning when spreadsheet is empty

3a24935

When downloading an empty spreadsheet, the content type of the downloaded file is "inode/x-empty" When shared configuration is misconfigured, the content type of the downloaded file is "text/html"

Fix #59 #67 : Add warning message when input is corrupted

d42175f

felginep requested review from jvigne and MrChoclate August 25, 2021 06:12

jvigne reviewed Aug 25, 2021

View reviewed changes

jvigne requested changes Aug 25, 2021

View reviewed changes

Use elsif instead of indented if

b1d460b

felginep added 2 commits August 26, 2021 09:53

Improve performance of ValueRangeToWording as well

7c8e881

Create KeyValidator to verify key integrity

e96c1a3

felginep force-pushed the feature/bugfix branch from 647934a to e96c1a3 Compare August 26, 2021 07:53

jvigne reviewed Aug 26, 2021

View reviewed changes

jvigne approved these changes Aug 26, 2021

View reviewed changes

Move KeyValidator to validators directory

702cd2a

jvigne approved these changes Aug 26, 2021

View reviewed changes

felginep merged commit 0c5cef4 into main Aug 26, 2021

felginep deleted the feature/bugfix branch August 26, 2021 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix #69

Bugfix #69

felginep commented Aug 25, 2021

jvigne Aug 25, 2021

felginep Aug 25, 2021

jvigne Aug 25, 2021

felginep Aug 25, 2021

jvigne Aug 25, 2021 •

edited

felginep Aug 25, 2021

jvigne Aug 25, 2021

jvigne Aug 25, 2021

jvigne Aug 25, 2021 •

edited

felginep Aug 25, 2021

jvigne Aug 25, 2021 •

edited

jvigne left a comment

felginep commented Aug 25, 2021

jvigne commented Aug 25, 2021

jvigne Aug 26, 2021

jvigne Aug 26, 2021

		existing_plural_key = existing_key.label == current_key.label && existing_key.plural? && current_key.singular?
		existing_singular_key = existing_key.label == current_key.label && existing_key.singular? && current_key.plural?

Bugfix #69

Bugfix #69

Conversation

felginep commented Aug 25, 2021

Fix #58

Fix #59 #67

Fix #61

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvigne Aug 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvigne Aug 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvigne Aug 25, 2021 • edited

Choose a reason for hiding this comment

jvigne left a comment

Choose a reason for hiding this comment

felginep commented Aug 25, 2021

jvigne commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvigne Aug 25, 2021 •

edited

jvigne Aug 25, 2021 •

edited

jvigne Aug 25, 2021 •

edited