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

Various issues with new sampledata module and plugins #17415

Closed
infograf768 opened this issue Aug 6, 2017 · 18 comments
Closed

Various issues with new sampledata module and plugins #17415

infograf768 opened this issue Aug 6, 2017 · 18 comments

Comments

@infograf768
Copy link
Member

infograf768 commented Aug 6, 2017

This new feature has just been merged. See #7680

My tests show multiple issues.

  1. A bug: the blog sampledata can be installed twice. It stops installing with an error when getting to menus, but it lets create articles and categories with same name and alias as the ones created in the first install. This although we have
if ($el.data('processed')) {
			alert(Joomla.JText._('MOD_SAMPLEDATA_ITEM_ALREADY_PROCESSED'));
			return;
		}

Exemple for categories:

screen shot 2017-08-06 at 09 10 23

The testing sample data can't be installed twice as we immediately get an error
Step 1 Failed: Another Tag has the same alias (remember it may be a trashed item).
But we should get the alert even before it tries to install imho.

  1. Another bug: it lets install another sample data after installing the first one. While Blog has been installed, one can also install the Testing sampledata. Imagine other plugins created and the mess it would create if any of the items has same name and alias in a similar hierarchy.

  2. When a site has been created as multilingual at installation time (Setting is in Installation : No sample data, and later one after installing languages, install localised content), it still lets install sampledata from the module, all data set to ALL languages from the ini of the language used in admin at that time.
    Although it does not break anything, it is totally useless on a multingual site and all data added has to be deleted, which is a real pain specially concerning Testing sample data.
    I imagine a possible further improvement would be to create a specific multilingual sample data plugin which would pick the inis from various installed languages and create a more complex multilingual test site, but, in the mean while I suggest to NOT be able to install ini-based sample datas when the language filter is enabled with a specific Alert.

  3. Improvements: The Alert displayed before installing the ini-based sample data should be more precise, i.e. stating that it is the data in the administrator language in use which is going to be installed and, if not present, default to en-GB.

@infograf768
Copy link
Member Author

Forgot also:
Shall we not inform the user at installation time that some sample data can be installed when login into administrator?
Is it useful to keep the various sql sample datas proposed at installation time?
It is quite confusing and language communities custom distros are directly impacted by this.

@Bakual
Copy link
Contributor

Bakual commented Aug 6, 2017

A bug: the blog sampledata can be installed twice. It stops installing with an error when getting to menus, but it lets create articles and categories with same name and alias as the ones created in the first install. This although we have

Not exactly a bug. The code will prevent you from installing the same dataset twice "accidentally" as long as you don't refresh the page (to prevent accidental double clicks). As soon as you refresh the page, it lets you install it again. It will fail as soon as it gets a duplicate alias in a category when a model complains.

I don't see a simple way yet to know in advance if a set is installed or not. And personally I don't think it's needed. It's not like something breaks if one installs the same set multiple times.

Another bug: it lets install another sample data after installing the first one. While Blog has been installed, one can also install the Testing sampledata. Imagine other plugins created and the mess it would create if any of the items has same name and alias in a similar hierarchy.

Not a bug but a feature. Imagine you have a sample data set for a specific extension.

When a site has been created as multilingual at installation time (Setting is in Installation : No sample data, and later one after installing languages, install localised content), it still lets install sampledata from the module, all data set to ALL languages from the ini of the language used in admin at that time.
Although it does not break anything, it is totally useless on a multingual site and all data added has to be deleted, which is a real pain specially concerning Testing sample data.

Not a bug but a feature. It may be useless for the shipped sample data in your eyes, for someone else it may be useful. Anyway it is easy to remove since the data is all in its own categories. It's a bit of work to remove again, yes, but it's not like we're installing it by default.

I imagine a possible further improvement would be to create a specific multilingual sample data plugin which would pick the inis from various installed languages and create a more complex multilingual test site

Yes, that could be done. But not pick from the various installed languages, instead ship its own INI files.

I suggest to NOT be able to install ini-based sample datas when the language filter is enabled

I don't understand why it should be disabled. It works as expected also in multilingual context. From memory the added data is added with the currently active language tag (if not, that would be simple to do) and if translated strings are available, will install the translated sample data. It will not associated them thought, that would be much more complex.

Improvements: The Alert displayed before installing the ini-based sample data should be more precise, i.e. stating that it is the data in the administrator language in use which is going to be installed and, if not present, default to en-GB.

It's a language string and thus easy to do. You want to do a PR for that?

Shall we not inform the user at installation time that some sample data can be installed when login into administrator?

The Control Panel is the first thing he will see after the installation is done. The module is present there for that reason. So he should see it.

Is it useful to keep the various sql sample datas proposed at installation time?

No, but due to B/C we have to keep them. We will (hopefully) drop them from J4. It's the main reason for that new sample data feature so we don't have to keep those SQLs (we had various issues with them in the past).

It is quite confusing and language communities custom distros are directly impacted by this.

In J3, the custom distros will work as they did before. For J4 I guess they need to be adjusted to sample data sets.

@zero-24
Copy link
Contributor

zero-24 commented Aug 6, 2017

I don't see a simple way yet to know in advance if a set is installed or not

Save the installed thing to the database ;)

@mbabker
Copy link
Contributor

mbabker commented Aug 6, 2017

I don't see a simple way yet to know in advance if a set is installed or not

Save the installed thing to the database ;)

Why should we need to track in the database what sample data set(s) the user has installed? What does it matter if we're tracking it?

@Bakual
Copy link
Contributor

Bakual commented Aug 6, 2017

Save the installed thing to the database ;)

And then what are you doing if the user removes the sample set and wants to install it again? Oops.

That's what I meant with "simple". We would have to run queries to check which is installed and which is not. Each plugin will have to query for the data it inserts and return based on the result. But should it only check a few items or all? What if the install is incomplete for some reason?
You open a whole new amount of possible issues. And in the end it just doesn't matter if a sample set is installed twice. You get rubbish data, but it will not break anything and you can remove it again afterwards.

@zero-24
Copy link
Contributor

zero-24 commented Aug 6, 2017

Why should we need to track in the database what sample data set(s) the user has installed? What does it matter if we're tracking it?

$params->set('sampledataset', 'XXX'); No extra database ;)

And then what are you doing if the user removes the sample set and wants to install it again? Oops.

Give them a button to clear the value ;)

else I guess we have just the chance to guess the installed sampledataset ;)

@mbabker
Copy link
Contributor

mbabker commented Aug 6, 2017

$params->set('sampledataset', 'XXX'); No extra database ;)

It's still storing the data in the database. I'm questioning why we even need this type of tracking capability to begin with, not where to put it in the database.

Give them a button to clear the value ;)

else I guess we have just the chance to guess the installed sampledataset ;)

Neither of those are good answers. To reverse the install is potentially destructive, especially if a user starts editing the generated content, or even worse has a site structure that might actually match the sample data set.

Once installed, it doesn't matter what set(s) are installed or how many times you try to install them.

@infograf768
Copy link
Member Author

A bug: the blog sampledata can be installed twice. It stops installing with an error when getting to menus, but it lets create articles and categories with same name and alias as the ones created in the first install. This although we have

Not exactly a bug. The code will prevent you from installing the same dataset twice "accidentally" as long as you don't refresh the page (to prevent accidental double clicks). As soon as you refresh the page, it lets you install it again. It will fail as soon as it gets a duplicate alias in a category when a model complains.

Once installed, it doesn't matter what set(s) are installed or how many times you try to install them.

I would not say it does not matter when we get the same alias multiple times for an article and a category.

sampledata

@infograf768
Copy link
Member Author

This would already help (after proper English corrections)

screen shot 2017-08-07 at 10 51 00

Note: to correct Testing Sampledata to Testing Sample Data both in title and description.

@infograf768
Copy link
Member Author

infograf768 commented Aug 7, 2017

I don't understand why it should be disabled. It works as expected also in multilingual context. From memory the added data is added with the currently active language tag (if not, that would be simple to do) and if translated strings are available, will install the translated sample data. It will not associated them thought, that would be much more complex.

When one has installed Joomla as multilingual, the sample datas proposed (ini translated or not for the admin language in use) are NOT multilingual aware. All is tagged to ALL languages which is totally useless and confusing for someone starting a multilingual site.

NOTE: BTW, Parks articles and category (EDIT: Banners too) are tagged to en-GB which is useless and totally obsolete. The reason from this early remain of 1.6 was that Elin thought that we could test the switcher while the site was not totally multilingual. It has never been corrected since. Good time to do it.

@Bakual
Copy link
Contributor

Bakual commented Aug 7, 2017

The duplicate alias in the category looks like a bug in the category model. I would expect either the model or table to check that. Not sure why it doesn't.
The duplicate article alias however is not an issue since they are in different categories and thus the same alias is allowed again.

As for the added notice, I'm not that sure. First the model only shows on new installations, not on updated ones. Obviously, you usually don't want to install sampe data sets on a productive site, but there may reasons to do it. Not the testing or blog one but possible 3rd party ones could be useful (eg if you're trying a new extension).

I still don't see the issue you have when in a multilingual site. Installing a sample data set on such a site is perfectly fine. Yes, it doesn't tag the active language. If you want, we can change it so it installs in the active language instead of "All", this would actually allow to install it multiple times (assuming you switch language between and the set is translated).
That would be a simple change,

Regarding the items tagged as en-GB, I actually just copied the existing set. There are other issues like content referring to no-longer existing templates and such things. Issues like that should be much easier to solve now.

@infograf768
Copy link
Member Author

Obviously, you usually don't want to install sampe data sets on a productive site, but there may reasons to do it. Not the testing or blog one but possible 3rd party ones could be useful (eg if you're trying a new extension).

We can change the message to use "Do not install Core distributed Sample Data etc."

The duplicate alias in the category looks like a bug in the category model. I would expect either the model or table to check that. Not sure why it doesn't.
The duplicate article alias however is not an issue since they are in different categories and thus the same alias is allowed again.

Indeed, but if the category part was doing its job correctly, the blog sample data would behave like it does for testing sample data, i.e. throwing an error at first try to install again.
It looks like the error to be used is in the store method in /libraries/src/Joomla/CMS/Table/Category.php
while for tags it is in the /administrator/components/com_tags/tables/tag.php
Therefore it breaks immediately when trying to save tags when reinstalling the testing sample data while it breaks AFTER saving categories and articles in blog sample data.

In TagsModelTag we have

if (!$table->store())
		{
			$this->setError($table->getError());

			return false;
		}

Therefore using $categoryModel->save($category); does not throw the error.
I am sure you can solve this easily.

@Bakual
Copy link
Contributor

Bakual commented Aug 7, 2017

We can change the message to use "Do not install Core distributed Sample Data etc."

Don't like it. How would you tell a core sample set apart from a 3rd party? And does that distinguishment even make sense?

Therefore it breaks immediately when trying to save tags when reinstalling the testing sample data while it breaks AFTER saving categories and articles in blog sample data.

Therefore it breaks immediately when trying to save tags when reinstalling the testing sample data while it breaks AFTER saving categories and articles in blog sample data.

I'm not sure. When I try to create a category in the backend with an existing alias, it fails with the expected error without creating the category, but in the case of the sample data it is created. The code used is in both cases the save method of the category model. At least that's what I think. Something must be different 😄

@infograf768
Copy link
Member Author

Don't like it. How would you tell a core sample set apart from a 3rd party?

We just have to modify the title/description in the plugins, for example Blog Sample Data (Core)

@Bakual
Copy link
Contributor

Bakual commented Aug 9, 2017

Don't see the reason for it. But if you want to propose a PR, go ahead.

@infograf768
Copy link
Member Author

Will make a proposal.

@infograf768
Copy link
Member Author

See #17456

@brianteeman
Copy link
Contributor

Is this still an open issue or has it been solved elsewhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants