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

[New Feature] Multilingual: Propagating existing associations if desired #21321

Merged
merged 8 commits into from
Aug 4, 2018

Conversation

infograf768
Copy link
Member

@infograf768 infograf768 commented Jul 31, 2018

Pull Request for Issue #21250

Summary of Changes

This adds a "Propagate" button in the Associations tab when editing a contact, category, article, menu item, newsfeed.
This button will display when an association is selected in a Content Language field, offering the option to Propagate the associations defined for that item to the other Content Languages.

The button will display ONLY when there are more than 2 Content Languages defined for the site.
A message will display with the results obtained.

It is B/C as it will not break anything and one is totally free to choose or not to use the button, thus keeping the possibility to Clear/Select/Create any associations as before.

Using it prevents breaking existing associations when the wrong items are chosen for some fields.

A big thank you to Robbie Jackson for the long work to get this new functionality.
Target Milestone: 3.9.0

Testing Instructions

Create a Multilingual site with 3 or more languages.
Create items in each type: (contact, category, article, menu item, newsfeed) for 2 of these languages and associate them in each case.

Create a new item for each type, select another language than the ones which are already associated.
Save (NOT save and close) this item.
Display the Association tab.

You will see, as usual (here for 4 Content Languages)

screen shot 2018-07-31 at 11 13 04

I do have already associations set for English and French.

I select for English the item which I know is associated.

The button displays (Better tip welcome)

screen shot 2018-07-31 at 11 17 17

Clicking on the button will give

screen shot 2018-07-31 at 11 18 22

Result: the already associated category in French is added in the French field.
As we have no association set for Spanish, the field remains empty.
The message displays (it is the same in all cases except if there is an Ajax error)
All existing associations have been set. If some are missing you may have to select or create them manually.

If we have already associated all fields, clicking on Propagate will just display the message.

If we change an association in one of the fields and click Propagate, it is the associations from that language item that will be propagated and therefore replace the ones existing.

Documentation Changes Required

Yes

@infograf768
Copy link
Member Author

the json files need corrections for cs. Can be tested and commented nevertheless. ;)

JGLOBAL_ASSOCIATIONS_PROPAGATE_BUTTON="Propagate"
JGLOBAL_ASSOCIATIONS_PROPAGATE_FAILED="Failed propagating associations. You may have to select or create them manually."
JGLOBAL_ASSOCIATIONS_PROPAGATE_MESSAGE="All existing associations have been set. If some are missing you may have to select or create them manually."
JGLOBAL_ASSOCIATIONS_PROPAGATE_TIP="Propagates this selected item existing associations."
Copy link
Contributor

Choose a reason for hiding this comment

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

Please correct to

"Propagates this item's existing associations."

@@ -281,6 +281,10 @@ JGLOBAL_ARTICLE_ORDER_DESC="The order that articles will show in."
JGLOBAL_ARTICLE_ORDER_LABEL="Article Order"
JGLOBAL_ARTICLES="Articles"
JGLOBAL_ASSOC_NOT_POSSIBLE="To define associations, please make sure the item language is not set to 'All'."
JGLOBAL_ASSOCIATIONS_PROPAGATE_BUTTON="Propagate"
JGLOBAL_ASSOCIATIONS_PROPAGATE_FAILED="Failed propagating associations. You may have to select or create them manually."
JGLOBAL_ASSOCIATIONS_PROPAGATE_MESSAGE="All existing associations have been set. If some are missing you may have to select or create them manually."
Copy link
Contributor

Choose a reason for hiding this comment

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

How likely is it that an association will be missing? Seems odd to me to have that on a success message.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will happen when there is no item associated in a specific language. See image above where there is nothing for Spanish

Copy link
Contributor

Choose a reason for hiding this comment

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

so in that case nothing is missing as it doesnt exist
The first sentence is more than enough

Copy link
Member Author

@infograf768 infograf768 Jul 31, 2018

Choose a reason for hiding this comment

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

This message is also informing the user that an association for one or more language(s) may be missing. It may be what the user wants but it also may not be.
In that case the user can select an item or create one directly in this interface.
This is important.

Copy link
Contributor

Choose a reason for hiding this comment

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

thats not the role of a success message.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is only a success message for what could be propagated.

@@ -281,6 +281,10 @@ JGLOBAL_ARTICLE_ORDER_DESC="The order that articles will show in."
JGLOBAL_ARTICLE_ORDER_LABEL="Article Order"
JGLOBAL_ARTICLES="Articles"
JGLOBAL_ASSOC_NOT_POSSIBLE="To define associations, please make sure the item language is not set to 'All'."
JGLOBAL_ASSOCIATIONS_PROPAGATE_BUTTON="Propagate"
JGLOBAL_ASSOCIATIONS_PROPAGATE_FAILED="Failed propagating associations. You may have to select or create them manually."
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be "will" not "may"?

Copy link
Member Author

@infograf768 infograf768 Jul 31, 2018

Choose a reason for hiding this comment

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

Some one may not want to create associations for a specific language.
Change to ?
If desired, you will have to select or create them manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

then they havent failed and this isnt a failure message

Copy link
Member Author

Choose a reason for hiding this comment

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

Propagate has failed (for an unknown reason. It may be the case for a 3rd party component which is badly coded). It does not mean the user lost the possibility to associate items by selecting/creating them.

I am now waiting for tests. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok change to your suggestion of "If desired, you will have to select or create them manually.

@brianteeman
Copy link
Contributor

brianteeman commented Jul 31, 2018

Testing this and my only issue is with the messages

The propagate button says

Propagate this items existing associations

But you always get this message

All existing associations have been set. If some are missing you may have to select or create them manually.

The code should check to see if there any associations to propagate and if not then the message should be

No associations exist to propagate

And if there are associations to propagate then the message should be

All existing associations have been set.

@infograf768
Copy link
Member Author

infograf768 commented Jul 31, 2018

We will now get 3 types of messages:
JGLOBAL_ASSOCIATIONS_PROPAGATE_MESSAGE_ALL="All existing associations have been set."
JGLOBAL_ASSOCIATIONS_PROPAGATE_MESSAGE_NONE="No associations exist to propagate."
JGLOBAL_ASSOCIATIONS_PROPAGATE_MESSAGE_SOME="Some associations have been set: %s"

JGLOBAL_ASSOCIATIONS_PROPAGATE_MESSAGE_SOME will also display the language(s) where associations have been added.

Example. Here I selected an article for English. It added the French existing association, but as the Spanish one did not exist, it is that message which is used displaying which association was set.
screen shot 2018-07-31 at 16 34 41

@infograf768
Copy link
Member Author

Restarted drone who failed on js.

JGLOBAL_ASSOCIATIONS_PROPAGATE_FAILED="Failed propagating associations. You may have to select or create them manually."
JGLOBAL_ASSOCIATIONS_PROPAGATE_MESSAGE_ALL="All existing associations have been set."
JGLOBAL_ASSOCIATIONS_PROPAGATE_MESSAGE_NONE="No associations exist to propagate."
JGLOBAL_ASSOCIATIONS_PROPAGATE_MESSAGE_SOME="Some associations have been set: %s"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change to
Associations have been set for: %s

@brianteeman
Copy link
Contributor

Much better now.

I am also getting a missing string JGLOBAL_ASSOCIATIONS_PROPAGATE_TIP

@infograf768
Copy link
Member Author

done

@jreys
Copy link
Contributor

jreys commented Aug 1, 2018

I have tested this item ✅ successfully on eabc256


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21321.

@infograf768
Copy link
Member Author

One more tester.

$associations[$lang]->title = $categoryTable->title;
}

$message = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove. Not necessary as it will be assigned later in the if statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Waiting for other comments from you before changing. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Repeat this change in the other files.

@@ -169,6 +176,23 @@ function jSelectNewsfeed_" . $this->id . "(id, title, object) {
. '</a>';
}

// Propagate contact button
Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment.

@infograf768
Copy link
Member Author

@Quy
All done. Thanks.

@infograf768
Copy link
Member Author

@Quy @franz-wohlkoenig @brianteeman
Please add your tests.

@ghost
Copy link

ghost commented Aug 1, 2018

@infograf768 have read Instructions a few Times but didn't understand really what is to test. So i need Time.

@ghost
Copy link

ghost commented Aug 2, 2018

I have tested this item ✅ successfully on b902d37


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21321.

@ghost
Copy link

ghost commented Aug 2, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 2, 2018
@infograf768
Copy link
Member Author

@mbabker
I have tagged this to 3.9. Can you merge?

@@ -101,6 +101,7 @@
<?php echo $this->form->getInput('extension'); ?>
<input type="hidden" name="task" value="" />
<input type="hidden" name="forcedLanguage" value="<?php echo $input->get('forcedLanguage', '', 'cmd'); ?>" />
<?php echo '<input id="token" type="hidden" name="' . JSession::getFormToken() . '" value="1" />'; ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

This line shouldn't be necessary, this is exactly what the JHtml call immediately below does.

@@ -120,5 +120,6 @@
</div>
<input type="hidden" name="task" value="" />
<input type="hidden" name="forcedLanguage" value="<?php echo $input->get('forcedLanguage', '', 'cmd'); ?>" />
<?php echo '<input id="token" type="hidden" name="' . JSession::getFormToken() . '" value="1" />'; ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@@ -138,6 +138,7 @@
<input type="hidden" name="task" value="" />
<input type="hidden" name="return" value="<?php echo $input->getCmd('return'); ?>" />
<input type="hidden" name="forcedLanguage" value="<?php echo $input->get('forcedLanguage', '', 'cmd'); ?>" />
<?php echo '<input id="token" type="hidden" name="' . JSession::getFormToken() . '" value="1" />'; ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@@ -232,6 +232,7 @@

<input type="hidden" name="task" value="" />
<input type="hidden" name="forcedLanguage" value="<?php echo $input->get('forcedLanguage', '', 'cmd'); ?>" />
<?php echo '<input id="token" type="hidden" name="' . JSession::getFormToken() . '" value="1" />'; ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@@ -107,5 +107,6 @@
</div>
<input type="hidden" name="task" value="" />
<input type="hidden" name="forcedLanguage" value="<?php echo $input->get('forcedLanguage', '', 'cmd'); ?>" />
<?php echo '<input id="token" type="hidden" name="' . JSession::getFormToken() . '" value="1" />'; ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

var currentLang = jQuery('#jform_language').find(":selected").val();

// Find the token so that it can be sent in the Ajax request as well
var token = jQuery("#token").attr("name");
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of finding the token in this way, the API added in #14952 should be used instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean using
var token = jQuery('meta[name=csrf-token]').val(); instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, that does not work

Copy link
Contributor

Choose a reason for hiding this comment

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

In the layouts, you have to add a JHtml::_('jquery.token'); call, then you don't even need to find the token and manually include it when using jQuery.ajax because that PHP call will set up jQuery to include the token automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

This works.
var token = Joomla.getOptions('csrf.token', '');

Is that OK, @mbabker ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

Copy link
Member Author

@infograf768 infograf768 Aug 2, 2018

Choose a reason for hiding this comment

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

I did not add the <?php echo JHtml::_('jquery.token'); ?>
Just deleted the extraneous lines in the edit.php and used
var token = Joomla.getOptions('csrf.token', '');

As I did not know what would be the consequences

Can we go with that?

@infograf768
Copy link
Member Author

@mbabker
Do we need new tests?

*
* @since __DEPLOY_VERSION__
*/
window.injectAssociations = function(result, callbackFunctionPrefix)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really bad. Please name this as Joomla.injectAssociations. Global variables, functions etc are as bad in Javascipt as they are in PHP 👎

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess Robbie used this as we have above existing
window.hideAssociation
and
window.showAssociationMessage

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I found what has to be modifed to fit as both the functions use window.
It is not the purpose of this patch to also correct the existing ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dgrammatiko
Do you agree with my statement?
If yes, I will do the patch right now

@infograf768
Copy link
Member Author

@dgrammatiko
Modified the new js functions name as required.
We can't obviously normalize the others as it would not be B/C.

@franz-wohlkoenig @jreys
Please test again. :)

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.9.0 milestone Aug 4, 2018
@ghost
Copy link

ghost commented Aug 4, 2018

I have tested this item ✅ successfully on 888a280


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21321.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 4, 2018
@infograf768 infograf768 added this to the Joomla 3.9.0 milestone Aug 4, 2018
@infograf768
Copy link
Member Author

@mbabker Can you now merge?

@mbabker mbabker merged commit 8bacca9 into joomla:staging Aug 4, 2018
@infograf768
Copy link
Member Author

@mbabker
As it is a new feature, shall not it be merged in 3.9.0 ?

@mbabker
Copy link
Contributor

mbabker commented Aug 4, 2018

💩

mbabker added a commit that referenced this pull request Aug 4, 2018
mbabker added a commit that referenced this pull request Aug 4, 2018
@mbabker
Copy link
Contributor

mbabker commented Aug 4, 2018

Branch merged to 3.9-dev via 11153f7

@infograf768
Copy link
Member Author

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants