Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

SDPAP-7602: Add a URL Redirect editing improvements. #129

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

henrytranvan
Copy link

Jira
https://digital-vic.atlassian.net/browse/SDPAP-7602

Issue

The content author adds the site id manually by taxonomy term id.

Change
The content author can select the site from the list.

Screenshot 2023-03-29 at 12 41 37 pm

@christopher-hopper
Copy link
Contributor

@henrytranvan

Would like to see some additions:

  1. Change the text URL prefix - https://content-sdp.docker.internal/ - shown before the "Path:" field, when a user chooses a "Site name:" from the drop-down.

See Drupal AJAX forms https://www.drupal.org/docs/drupal-apis/javascript-api/ajax-forms

We want to give the user clearer feedback in the form when they choose a Site from the drop-down. No magic. Let the user know what is happening when a Site is chosen.

  1. Set the default "Site name:" to the matching the site-id from "Path:" when editing a URL Redirect.

If I edit an existing URL Redirect, I expect to see the "Site name" chosen already. We can match the "site-id" in the "Path:" and choose the correct "Site name:" as the default value.

  1. Some validation to detect (and possibly fix) when someone chooses a "Site name" but has also manually entered a "site-id" prefix in the "Path:" field.

Try to detect and either fix or warn a user when they choose a "Site name" from the drop-down but enter a "site-id" in the "Path:". We don't want the "Path:" being saved like this:

Path: /site-7/site-7/path-to-my-page

or

Path: /site-7/site-12/path-to-my-page

A simple validation should detect and prevent issues when the drop-down for Site name is used but someone has also got a "site-id" written in their redirect "Path" field.

tide_site.module Outdated

foreach ($term_tree as $term) {
if ($term instanceof Term) {
$options['site-' . $term->id()] = $term->getName();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use the Term Id as an integer for the options id.

$options[$term->id()] = $term->getName();

Will allow us to use the integer in validation and possibly AJAX calls to load the term by its id.

tide_site.module Outdated
}
}
asort($options, SORT_STRING);
$options = [...['none' => 'Select site'], ...$options];
Copy link
Contributor

Choose a reason for hiding this comment

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

Change none to 0 for the first Site in the drop-down. Use an integer.

Change the Select site to None or - (dash). If a user chooses to not select a site, they're wanting to create a redirect for the CMS. This is allowed. You might want to redirect from a Drupal admin page to another Drupal admin page, so it is okay to not "Select site" but just use the default of no site.

tide_site.module Outdated
if ('redirect_redirect_form' === $form_id) {
$sites_term = 'sites';
// Get all primary sites ony, do not load sub-sites.
$term_tree = \Drupal::entityTypeManager()->getStorage('taxonomy_term')->loadTree(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we're breaking this over multiple lines, it aids readability if you put line breaks between the chained method calls, rather than in the params. I found this a bit hard to read and initially confusing.

I would suggest this is easier to read and follow for me:

    // Get the primary Sites only. Do not load child Sites sections.
    $vocabulary = 'sites';
    $term_tree = \Drupal::entityTypeManager()
      ->getStorage('taxonomy_term')
      ->loadTree($vocabulary, 0, 1, TRUE);

tide_site.module Outdated
@@ -588,6 +588,40 @@ function tide_site_form_alter(array &$form, FormStateInterface $form_state, $for
}
}
}
// Add site prefix to redirect URL.
if ('redirect_redirect_form' === $form_id) {
$sites_term = 'sites';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename this variable as $vocabulary or $taxonomy for clarity. It's not a "term" so could be a bit confusing or unclear when we read the code. This variable contains the name of the Vocabulary or Taxonomy we want to load from.

Alternatively, don't even store this in a variable at all. If we're not using it more than once, we don't need to use the memory allocation of a variable at all.

@henrytranvan henrytranvan force-pushed the feature-SDPAP-7602-add-URL-redirect branch from 35f753e to 7c0e7b5 Compare May 29, 2023 09:27
@henrytranvan henrytranvan force-pushed the feature-SDPAP-7602-add-URL-redirect branch from 8dc5863 to bdd3237 Compare May 29, 2023 09:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants