Add diacritics setting #1636

melaniekung · 2023-07-12T17:18:03Z

New setting for users to enable further char_filter mappings for diacritics, assigned to the default, autocomplete, english and french analyzers. Requires a yaml file to enable the setting, example attached to this gist for reference: https://gist.github.com/melaniekung/6014e100767019b1f85637e5a7cdf9df. Commit includes an empty diacritics_mapping.yml (default) which will be replaced with the submitted yaml to map out the required diacritics.

mcantelon

I left some feedback, but looks good overall!

mcantelon · 2023-07-13T02:04:16Z

plugins/arElasticSearchPlugin/lib/arElasticSearchPlugin.class.php

+            ));
+
+            if (!count($diacriticsFiles)) {
+                throw new sfException('You must create a diacritics_mapping.xml file.');


Should the "xml" in this line be "yml"?

mcantelon · 2023-07-13T02:17:50Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+            $this->form->bind($request->getPostParameters(), $request->getFiles());
+
+            if ($this->form->isValid()) {
+                if (0 === intval($request->getPostParameters()['diacritics']) || (1 === intval($request->getPostParameters()['diacritics']) && 0 !== $request->getFiles()['mappings']['size'])) {


Nitpick: Instead of 0 === intval you could also use empty"for the check (and insted of 1 === intval you could use empty.

mcantelon · 2023-07-13T02:24:48Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+
+        if (1 === (int) $request->getPostParameters()['diacritics']) {
+            if ('application/x-yaml' === $request->getFiles()['mappings']['type']) {
+                // Move uploaded file to new location to pass off to background arFileImportJob.


Is this handled by a background job? I don't see that in the cde.

mcantelon · 2023-07-13T02:25:41Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+                }
+
+                // Replace /config/diacritics_mapping.yml with uploaded file
+                $diacriticsMappingPath = '/atom/src/plugins/arElasticSearchPlugin/config/diacritics_mapping.yml';


Should this be an absolute path given that where AtoM's code lives may vary? Also I'm wondering if the config dir in the root of the AtoM installation might be better given that files in plugin conf directories usually don't get changed.

is there a way to get AtoM's base code location instead of using /atom/src/? i chose to keep it in arElasticSearchPlugin/config since that's where mapping.yml is located and I reused some of the code for that in arElasticSearchPlugin.class.php, but could change that if you think it's better.

Yeah, you might be right about putting it in the plugin directory so I'd say go with that (I'm a bit fuzzy on Symfony's config best practices, tbh).

To get the plugin's config directory path dynamically you can do:

sfConfig::get('sf_plugins_dir').DIRECTORY_SEPARATOR.'arElasticSearchPlugin'.DIRECTORY_SEPARATOR.'config';

mcantelon · 2023-07-13T02:31:41Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+            }
+        } else {
+            // Reset diacritics yaml when disabling the setting
+            $diacriticsMappingPath = '/atom/src/plugins/arElasticSearchPlugin/config/diacritics_mapping.yml';


This variable could maybe be set earlier in code as it ends up being set to the same thing twice.

mcantelon · 2023-07-13T02:34:12Z

apps/qubit/modules/settings/templates/diacriticsSuccess.php

+<?php slot('content'); ?>
+
+  <div class="alert alert-info">
+    <p><?php echo __('Please rebuild the search index if you are using diacritics mappings.'); ?></p>


Good thinking to add this as a reminder given that folks may miss the flash message about it. Maybe specify that this needs to be done after the mapping is uploaded.

mcantelon · 2023-07-13T02:35:03Z

apps/qubit/modules/settings/templates/diacriticsSuccess.php

+          <tr>
+            <td><?php echo __('Mappings YAML'); ?></td>
+            <td>
+              <span><?php echo 'YAML file containing char_filter mappings.'; ?></span></br>


Should the text being echoed here be wrapped in a call to the __ function?

mcantelon · 2023-07-13T02:37:38Z

data/fixtures/settings.yml

@@ -3,7 +3,7 @@ QubitSetting:
    name: version
    editable: 0
    deleteable: 0
-    value: 193


I don't know if a migration's actually needed for this as I think your call to parent::earlyExecute(); will end up creating the new setting in the DB.

initially, we started out with a global setting so we created a migration. if it's not needed, what changes do i need to make?

I'd try using the setting in a fresh install of AtoM without running the migration and see if it works. If it works without the migration then all you need to do is remove the changes settings.yml from your dev change (and remove the actual migration file).

nice, thanks mike! i think all the mentioned issues have been resolved now

Nicely done! Looks good!

mcantelon · 2023-07-13T02:43:36Z

plugins/arElasticSearchPlugin/lib/arElasticSearchPlugin.class.php

@@ -117,6 +123,8 @@ public function __destruct()

    public static function loadMappings()
    {
+        global $globalDiacritics;


Could you just use the protected $globalDiacritics property declared earlier?

since it's a static function, i can't use $this so i'm not sure how else to access $globalDiacritics, suggestions?

It seems like something like this could work:

Move "Check for diacritics_mapping.yml" code into a new method (called loadCharMapping or loadCharFilter whatever makes sense)

Have this method return the results of sfYaml::load

In the initialize method set $this->config['index']['configuration']['analysis']['char_filter']['diacritics_lowercase'] to the results of a call to this method

mcantelon

Looking good! I've added a bit of feedback, but nothing major!

mcantelon · 2023-07-13T23:08:59Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+                    $this->uploadDiacritics($request);
+
+                    $this->getUser()->setFlash('notice', $this->context->i18n->__('Diacritics setting updated. Rebuild the search index for the changes to be applied.'));
+                    $this->redirect(['module' => 'settings', 'action' => 'diacritics']);


The same redirect is done in both if conditions. You could do it after the if statement.

mcantelon · 2023-07-13T23:10:32Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+            }
+        } else {
+            // Reset diacritics yaml when disabling the setting
+            file_put_contents($diacriticsMappingPath, '');


Maybe just delete the file via unlink?

mcantelon · 2023-07-13T23:12:52Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+                try {
+                    $file = Qubit::moveUploadFile($file);
+                } catch (sfException $e) {
+                    $this->getUser()->setFlash('error', $e->getMessage());


It might be better, for security reasons, to provide a generic error like "Error: unable to upload" or some such thing. Otherwise the error message might divulge system information.

It should also return after setting the flash, right?

mcantelon · 2023-07-13T23:14:06Z

data/fixtures/settings.yml

@@ -1351,6 +1351,11 @@ QubitSetting:
  Qubit_Settings_defaultArchivalDescriptionBrowseView:
    name: default_archival_description_browse_view
    value: table
+  Qubit_Settings_diacritics:


You shouldn't need to add anything to this file. I think the setting will be dynamically created no matter what.

if i remove these lines, i get a 500 error when i try to open up the diacritics settings page.

Ah weird... I'll take a look.

Can you try just changing line 33 to "diacritics" => "0"?

(Hmm that doesn't seem to work - gets rid of 500 error but causes other issue. I'll poke around more.)

hmm were you able to upload a yaml file with those changes? i get the following error when uploading:

atom_1 | [18-Jul-2023 17:25:16] WARNING: [pool atom] child 15 said into stderr: "NOTICE: PHP message: This form is multipart, which means you need to supply a files array as the bind() method second argument."

and this error was the reason why I initially used DefaultEditAction::execute($request); as opposed to parent::execute($request);

I was able to upload... I did run into that error but there was a change I made that resolved that. It's entirely possible there was a change I made that I forgot to add to the list, though, so I'll re-create the changes to see if I missed something!

Yeah, I think I tweaked apps/qubit/modules/settings/actions/editAction.class.php to get it to work and forgot about it. Changing line 31 of that file to $this->form->bind($request->getPostParameters(), $request->getFiles()); fixes things (basically just adding the second argument in the function call so the SettingsEditAction can work with multipart form data). So I'd say include that fix in your PR as well.

ah right, i've tried that before but then it runs the parent functions and skips uploadDiaricits(), which is where all my error handling is, i'll play around more and see if there's another way for it to run the function! thanks mike!

yay okay, i found a solution to that! lmk what you think of this solution!

mcantelon

Getting close! Thanks for your patience!

mcantelon · 2023-07-18T02:00:58Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+                $uploadMappingPath = $file['tmp_name'];
+                $diacriticsMappings = file_get_contents($uploadMappingPath);
+                file_put_contents($diacriticsMappingPath, $diacriticsMappings);
+            }


Might be good to put an else in this logic to flash an error at the user if file isn't "application/x-yaml".

thank you for your patience with my work XD i also updated diacriticsSuccess.php to make the save button match those in BS5, but that changed the look on BS2 theme. any suggestions on making them match other save buttons in both themes?

I'm not too up-to-speed on BS5 but I'll take a look!

i added a success page to the BS5 theme plugin to resolve this issue!

Might be good to put an else in this logic to flash an error at the user if file isn't "application/x-yaml".

If setting the mine type in the sfValidatorFile, it will work like a normal form error.

lib/task/migrate/migrations/arMigration0193.class.php

plugins/arElasticSearchPlugin/lib/arElasticSearchPlugin.class.php

mcantelon · 2023-07-24T21:19:11Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+    public function uploadDiacritics($request)
+    {
+        $file = $request->getFiles('mappings');
+        $diacriticsMappingPath = sfConfig::get('sf_plugins_dir').DIRECTORY_SEPARATOR.'arElasticSearchPlugin'.DIRECTORY_SEPARATOR.'config'.DIRECTORY_SEPARATOR.'diacritics_mapping.yml';


Could be good to make this multi-line... something like:

arElasticSearchPlugin' . DIRECTORY_SEPARATOR . 'config' . DIRECTORY_SEPARATOR . 'diacritics_mapping.yml';```

mcantelon · 2023-07-24T21:22:22Z

It's looking great... on the home stretch!

Sorry for not looking at the BS5 earlier, but seems you worked out a nice fix.

There's a couple minor issues left (including one nitpick I just added) but seems, unless @jraddaoui spots anything, like it should be good to go after that:

melaniekung · 2023-07-25T15:37:21Z

It's looking great... on the home stretch!

Sorry for not looking at the BS5 earlier, but seems you worked out a nice fix.

There's a couple minor issues left (including one nitpick I just added) but seems, unless @jraddaoui spots anything, like it should be good to go after that:

https://github.com/artefactual/atom/pull/1636/files#r1272766735

https://github.com/artefactual/atom/pull/1636/files#r126313014

jraddaoui

Sorry, a bit late to the review of the form part and I still couldn't check the templates, but I have added a few comments.

apps/qubit/modules/settings/actions/diacriticsAction.class.php

jraddaoui · 2023-07-25T16:41:47Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+                try {
+                    $file = Qubit::moveUploadFile($file);
+                } catch (sfException $e) {
+                    $this->getUser()->setFlash('error', $e->getMessage());


It should also return after setting the flash, right?

apps/qubit/modules/settings/actions/diacriticsAction.class.php

jraddaoui · 2023-07-25T16:44:47Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+                $uploadMappingPath = $file['tmp_name'];
+                $diacriticsMappings = file_get_contents($uploadMappingPath);
+                file_put_contents($diacriticsMappingPath, $diacriticsMappings);
+            }


Might be good to put an else in this logic to flash an error at the user if file isn't "application/x-yaml".

If setting the mine type in the sfValidatorFile, it will work like a normal form error.

apps/qubit/modules/settings/actions/diacriticsAction.class.php

apps/qubit/modules/settings/actions/editAction.class.php

mcantelon

Almost there!

mcantelon · 2023-07-26T18:41:57Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+                $uploadMappingPath = $file['tmp_name'];
+                $diacriticsMappings = file_get_contents($uploadMappingPath);
+                file_put_contents($diacriticsMappingPath, $diacriticsMappings);
+            }


mcantelon · 2023-08-01T21:56:28Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+                // Reset diacritics settings when uploading yaml fails
+                QubitSetting::findAndSave('diacritics', 0, ['sourceCulture' => true]);
+                unlink($diacriticsMappingPath);
+                $this->getUser()->setFlash('error', $this->context->i18n->__('Error: Unable to upload diacritics mapping.'));


If you add a line unset($this->updateMessage); after the setFlash line then you should be good!

mcantelon

Looks good!

mcantelon · 2023-08-09T19:52:20Z

@jraddaoui How's this looking to you? I've approved but needs your okay to merge.

jraddaoui

Getting close! Some tweaks for the validation below, still looking at the B5 template.

apps/qubit/modules/settings/templates/diacriticsSuccess.php

apps/qubit/modules/settings/actions/diacriticsAction.class.php

jraddaoui · 2023-08-11T17:30:28Z

plugins/arElasticSearchPlugin/lib/arElasticSearchPlugin.class.php

+            $this->config['index']['configuration']['analysis']['char_filter']['diacritics_lowercase'] = $this->loadDiacriticsMappings();
+        }
+
+        $this->loadMappings();


Why do we need this extra mapping load?

this second mapping is for the mapping.yml, separate function than loadDiacriticsMapping,

atom/plugins/arElasticSearchPlugin/lib/arElasticSearchPlugin.class.php

Line 118 in 35d1a58

public static function loadMappings()

But why do we need to call it here? It's not assigning the returned value to anything and it will be called below through $this->loadAndNormalizeMappings(); before the mappings are really needed.

jraddaoui

For the B5 template:

plugins/arDominionB5Plugin/modules/settings/templates/diacriticsSuccess.php

jraddaoui

Thanks @melaniekung! Only the comment about $this->loadMappings(); left, and added a few nitpicks.

jraddaoui · 2023-08-14T15:00:16Z

apps/qubit/modules/settings/actions/diacriticsAction.class.php

+                        $this->getUser()->setFlash('error', $this->context->i18n->__('Unable to upload diacritis mapping yaml file.'));
+                        unset($this->updateMessage);
+
+                        return;


Nitpick: this return is no longer needed.

jraddaoui · 2023-08-14T15:01:31Z

plugins/arDominionB5Plugin/modules/settings/templates/diacriticsSuccess.php

+        <?php echo render_field(
+          $form->diacritics
+              ->label(__('Diacritics'))
+        ); ?>


Nitpick: it could fit in one line.

jraddaoui · 2023-08-14T15:01:45Z

plugins/arDominionB5Plugin/modules/settings/templates/diacriticsSuccess.php

+        <?php echo render_field($form->mappings
+            ->label(__('Mappings YAML'))); ?>


Nitpick: it could fit in one line.

jraddaoui · 2023-08-14T15:02:16Z

plugins/arDominionB5Plugin/modules/settings/templates/diacriticsSuccess.php

+
+</form>
+
+<?php end_slot(); ?>


Nitpick: missing empty line at the end.

@jraddaoui changes made :)

jraddaoui

Thanks @melaniekung, nice work!

Code looks good to me but now that I check I don't see a ticket/issue related to it. Make sure the @artefactual/atom-maintainers know about this feature before merging, as these changes will require documentation and deployment considerations (for the config/diacritics_mapping.yml file). As we chatted, a next iteration of this feature could involve moving the actual mapping into the DB (now that it even has a settings page), to avoid the use of the filesystem and configuration files.

melaniekung requested a review from a team July 12, 2023 17:23

melaniekung force-pushed the dev/diacritics branch from 711bf4a to 22672a3 Compare July 12, 2023 17:45

mcantelon requested changes Jul 13, 2023

View reviewed changes

melaniekung force-pushed the dev/diacritics branch 3 times, most recently from 35c1e92 to a20cde0 Compare July 13, 2023 21:32

mcantelon requested changes Jul 13, 2023

View reviewed changes

melaniekung force-pushed the dev/diacritics branch from a20cde0 to dd79435 Compare July 17, 2023 15:00

mcantelon requested changes Jul 18, 2023

View reviewed changes

melaniekung force-pushed the dev/diacritics branch from dd79435 to 8154e17 Compare July 18, 2023 14:50

jraddaoui reviewed Jul 18, 2023

View reviewed changes

lib/task/migrate/migrations/arMigration0193.class.php Outdated Show resolved Hide resolved

jraddaoui reviewed Jul 18, 2023

View reviewed changes

plugins/arElasticSearchPlugin/lib/arElasticSearchPlugin.class.php Outdated Show resolved Hide resolved

jraddaoui reviewed Jul 18, 2023

View reviewed changes

plugins/arElasticSearchPlugin/lib/arElasticSearchPlugin.class.php Outdated Show resolved Hide resolved

melaniekung force-pushed the dev/diacritics branch 8 times, most recently from d879e36 to 0110b3c Compare July 24, 2023 15:49

mcantelon reviewed Jul 24, 2023

View reviewed changes

melaniekung force-pushed the dev/diacritics branch from 0110b3c to a65210c Compare July 25, 2023 15:27

melaniekung closed this Jul 25, 2023

melaniekung reopened this Jul 25, 2023

jraddaoui requested changes Jul 25, 2023

View reviewed changes

melaniekung force-pushed the dev/diacritics branch 2 times, most recently from a2552d5 to ee4095c Compare July 28, 2023 00:07

mcantelon requested changes Aug 1, 2023

View reviewed changes

melaniekung force-pushed the dev/diacritics branch from ee4095c to 85149a7 Compare August 2, 2023 19:51

mcantelon approved these changes Aug 9, 2023

View reviewed changes

melaniekung force-pushed the dev/diacritics branch 2 times, most recently from 2f15fc1 to 35d1a58 Compare August 11, 2023 16:35

jraddaoui requested changes Aug 11, 2023

View reviewed changes

jraddaoui reviewed Aug 11, 2023

View reviewed changes

plugins/arDominionB5Plugin/modules/settings/templates/diacriticsSuccess.php Outdated Show resolved Hide resolved

melaniekung force-pushed the dev/diacritics branch from 35d1a58 to 377ab20 Compare August 11, 2023 19:53

jraddaoui reviewed Aug 14, 2023

View reviewed changes

Add diacritics setting

5904d7a

melaniekung force-pushed the dev/diacritics branch from 377ab20 to 5904d7a Compare August 14, 2023 15:28

jraddaoui approved these changes Aug 14, 2023

View reviewed changes

melaniekung merged commit 9dd0656 into qa/2.x Aug 14, 2023
6 checks passed

melaniekung deleted the dev/diacritics branch August 14, 2023 18:54

anvit added this to the 2.8.0 milestone Sep 15, 2023

melaniekung linked an issue Oct 20, 2023 that may be closed by this pull request

Add Diacritics setting funtionality #1684

Closed

		<?php echo render_field($form->mappings
		->label(__('Mappings YAML'))); ?>

Add diacritics setting #1636

Add diacritics setting #1636

Conversation

melaniekung commented Jul 12, 2023 • edited Loading

mcantelon left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcantelon Jul 13, 2023 • edited Loading

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

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

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

mcantelon left a comment

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

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

Choose a reason for hiding this comment

mcantelon Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

melaniekung Jul 18, 2023 • edited Loading

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

Choose a reason for hiding this comment

mcantelon left a comment

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

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

mcantelon commented Jul 24, 2023 • edited Loading

melaniekung commented Jul 25, 2023

jraddaoui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcantelon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcantelon left a comment

Choose a reason for hiding this comment

mcantelon commented Aug 9, 2023

jraddaoui left a comment

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

jraddaoui left a comment

Choose a reason for hiding this comment

jraddaoui left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jraddaoui left a comment

Choose a reason for hiding this comment

melaniekung commented Jul 12, 2023 •

edited

Loading

mcantelon Jul 13, 2023 •

edited

Loading

mcantelon Jul 17, 2023 •

edited

Loading

melaniekung Jul 18, 2023 •

edited

Loading

mcantelon commented Jul 24, 2023 •

edited

Loading