? Success

User tests: Successful: Unsuccessful:

avatar Bakual
Bakual
25 Nov 2016

This is a bigger refactoring of com_fields to get rid of the use of com_categories for its field groups (fieldset) feature.

During the rewrite I found also several code pieces which are unused or were copy-pasted from com_categories but don't make sense in the context of fields.
I also replaced the useage of some deprecated methods.

So forgive me that it become a bit bigger than initially planned 😄

Summary of Changes

  • Main thing is that the field groups are now handled in com_fields using own views.
  • ACL should work now at least better than before.

Testing Instructions

  • Make sure managing the fields and groups works
  • Also test that the fields appear as expected. You don't have to test the indivual field types as I haven't changed anything there.
  • Please also test ACL. It should inherit permissions from component to field group to field. I haven't had the time yet to test that myself but in theory the code should work.

Documentation Changes Required

If there already is documentation how to implement com_fields into an extension, that will need to be adjusted.

  • Needs a helper method which returns possible contexts.
  • Needs added permission sections in the access.xml file.
avatar Bakual Bakual - open - 25 Nov 2016
avatar Bakual Bakual - change - 25 Nov 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 25 Nov 2016
Category SQL Administration com_admin Postgresql com_categories com_contact com_content com_fields
avatar Bakual Bakual - change - 25 Nov 2016
Milestone Added:
avatar brianteeman
brianteeman - comment - 25 Nov 2016

Quick comment on fields in com_contact
There is a dropdown (as before) but it is showing more than it should - see screenshot
image

I dont believe it is supposed to be showing articles and users?

Also by default you open with contact as the context and a url of

administrator/index.php?option=com_fields&context=com_contact.contact

when you switch the context to mail in the select you get a url of

administrator/index.php?option=com_fields&view=fields

I expected

administrator/index.php?option=com_fields&context=com_contact.mail

avatar brianteeman
brianteeman - comment - 25 Nov 2016

Looking closer
I see this dropdown everywhere now. Before you only had it on the two context in mail

That in itself is NOT a problem
the problem is that when you switch context to a different component you dont really notice as the only thing that is changing on the screen is the title which is ABOVE and to the left of the select. From a UI perspective this is outside of your field of vision and its confusing

avatar Bakual
Bakual - comment - 25 Nov 2016

Actually that is intented behaviour, but something which can be discussed if we want it or not.
The dropdown would allow to switch between the extensions that support custom fields. It takes the contexts from the extensions helper method. The sidebar automatically adjusts to the selected extension.

The URL doesn't need the context/extension anymore after it is initially set. So if you click the link in the sidebar, you see the extension/context in the URL. It will then set the userstate and from there on it's not needed anymore. If you use the dropdown, the extension/context will be sent in the form (POST) and not in the URL (GET). So that is fine.

avatar Bakual
Bakual - comment - 25 Nov 2016

the problem is that when you switch context to a different component you dont really notice as the only thing that is changing on the screen is the title which is ABOVE and to the left of the select. From a UI perspective this is outside of your field of vision and its confusing

The sidebar also changes, but I agree it may not be noticed.

avatar brianteeman
brianteeman - comment - 25 Nov 2016

It is very confusing - it will lead to bug reports. It also prevents someone from direct linking to a field menu (unless you understand the secret urls)

fields

avatar Bakual
Bakual - comment - 25 Nov 2016

It also prevents someone from direct linking to a field menu (unless you understand the secret urls)

Direct links should work imho. The sidebar and menu links doen't do anything else and they work fine as far as I tested.
Not going to argue about the confusing part. I'm not good with UIs anyway.
I just kind of liked the possibility to switch fast between the extension and thought I add it. It's simple to remove again if we don't want it. We just have to adjust the formfield.
It would also be simple to adjust the options eg to "Contact -> Mail" instead of just "Mail" if that helps.

avatar brianteeman
brianteeman - comment - 25 Nov 2016

Direct links should work imho.

they will if you copy the one that has the full context in. They wont if you use the url you see when you use the switcher as seen in the video

avatar Bakual
Bakual - comment - 25 Nov 2016

Yep, that's true. Not sure how important it is. It may be a bit tricky to do and likely involves JavaScript if you want to change the URL from a select. I think in current staging it's done using JS.

avatar laoneo
laoneo - comment - 26 Nov 2016

IMO it will be confusing for the end user to have all of the sudden an option to switch between components. In Joomla we don't have such functionality nowhere, so I would not introduce it. I would also do it the way that when there is only one context in the component, that the field will be hidden at all.

avatar Bakual
Bakual - comment - 26 Nov 2016

In Joomla we don't have such functionality nowhere, so I would not introduce it.

To be fair, the only place where we currently have a similar cross-extension extension is com_categories. That one doesn't have such a select, that's true.
But that's not really an argument to me since we're only talking about one possible place.
The Google project for translating/associating would be another such extension and that one could use such a selector.

You could look at it as a required option to filter fields. Like the search filters but not optional.
But if all think it is to confusing I have no problem removing it (or make it an option - which brings up another topic fast g)

avatar brianteeman
brianteeman - comment - 26 Nov 2016

I would remove it.

avatar laoneo
laoneo - comment - 26 Nov 2016

I can't find a use case where you are working on custom fields for articles and then you need to switch to the custom fields of users.

avatar Bakual
Bakual - comment - 26 Nov 2016

I can think of use cases when you're setting up a site.
But please this PR isn't about this single select (I already said I can remove it if all don't like it).
Much more important would be to test the function of the PR. Eg that fields and groups still work as expected and also ACL which should now work properly with inheritance from component -> groups -> fields.

avatar Bakual
Bakual - comment - 26 Nov 2016

Removed the switcher from groups view and only show it in fields view when there is more than one context.

avatar ralain
ralain - comment - 27 Nov 2016

Some issues emptying the trash of field groups:

  1. ACL error.
  2. Page title broken and sidebar empty.
  3. Context missing from URL, simply ending in "extension=". I assume this is the cause of 1 and 2.
  4. The success message states that 1 field was deleted (it was a field group.)

screen shot 2016-11-27 at 17 45 49

avatar ralain
ralain - comment - 27 Nov 2016

Also wrong context com_content.article on attempting to create a field under com_users.

avatar Bakual
Bakual - comment - 27 Nov 2016

@ralain Thanks for the test, the first one is indeed because there is an empty "&extension=" in the URL. I have to see where that comes from. The other is strange.

avatar Bakual
Bakual - comment - 27 Nov 2016

@ralain I think I have found the culprits. Can you please test again?

avatar ralain
ralain - comment - 28 Nov 2016

Yep, I am now able to create fields under com_users, and apart from the "field deleted" message, the rest appears to be fixed as well.

avatar Bakual
Bakual - comment - 28 Nov 2016

Fixed the message as well.
Also found another wrong redirect which is now fixed.

avatar ralain
ralain - comment - 28 Nov 2016

Looks like there is a permissions issue on the Field Groups views. When logged in as a Manager I get a "You are not authorised to view this resource," regardless of ACL settings.

avatar Bakual
Bakual - comment - 29 Nov 2016

@ralain Should be fixed now.
Also found out that I forget to take care of the batch methods. Will need to do that.

avatar Bakual
Bakual - comment - 29 Nov 2016

Batch processing should work as well now.

avatar infograf768
infograf768 - comment - 30 Nov 2016

Could someone explain to me the usage of the language field, both in fields and fieldgroup?

My test show here (simple text field for article) that, on a multilingual site, tagging the field to a specific language (example fr-FR) displays the field in the article edit form both when the article is tagged to the same language (fr-FR) and when the article language is set to "All".

avatar Bakual
Bakual - comment - 30 Nov 2016

I think the field will only display if the language of the field matches the site language. Not related to the language of the article.
But I haven't changed anything in regards to that and I honestly have no clue if it makes sense to have that feature or not.

avatar infograf768
infograf768 - comment - 30 Nov 2016

In fact, I was first speaking of backend editing, not about the frontend display. This means we have a bug here.
@laoneo
Can you look into this?

avatar laoneo
laoneo - comment - 30 Nov 2016

I guess in fieldgroups it is not necessary. If the article you are editing belongs to a language, then the fields are only displayed which belong to the same language.

avatar infograf768
infograf768 - comment - 30 Nov 2016

Then we do have a bug as it also displays when language is set to ALL for the article when field is set to fr-FR.

EDITED

avatar laoneo
laoneo - comment - 30 Nov 2016

Should be or not? If a field should shown on all languages and an article as language fr-FR then this field should also be loaded, I guess or I'm wrong?

avatar infograf768
infograf768 - comment - 30 Nov 2016

THe case I describe is exactly the opposite
The field is set to a specific language and the article is set to ALL

avatar laoneo
laoneo - comment - 30 Nov 2016

Here is the line which defines it https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_fields/helpers/fields.php#L86. If the language is set to all on the article then all fields are loaded, which is right IMO. But you know better than me how the multilangual behavior should be.

avatar Bakual
Bakual - comment - 30 Nov 2016

Personally, I'd question the whole use of languages on the field and field groups.
Is there even a need for that or does it only complicate code and confuse users? It lookes like it even confuses me and JM already 😄
After all, you can already filter fields by category. Eg they only show if the article belongs to a specific category. On a multilingual site chances are high the category is language specific as well.

avatar brianteeman
brianteeman - comment - 30 Nov 2016

On a multilingual site chances are high the category is language specific
as well.

High but not required. It depends on how you build the site

avatar Bakual
Bakual - comment - 30 Nov 2016

High but not required. It depends on how you build the site

True. do you think there is a need for a language field on the field (and the groups)? If so, what would be a possible use case?
Just so we know how the code should work, because imho it's not that clear. The language filter could either depend on the language of the item (eg article) or depend on site language. Currently it is depending on the item language (if I understood @laoneo correct). But that becomes interesting in the edit form since you can change the language of the item there.

avatar dgt41
dgt41 - comment - 30 Nov 2016

But that becomes interesting in the edit form since you can change the language of the item there.

We can always have something like #11040 and overcome the client side changes...

avatar brianteeman
brianteeman - comment - 30 Nov 2016

True. do you think there is a need for a language field on the field (and
the groups)? If so, what would be a possible use case?

Scenario 1
French Music group - this group is only for french language content

Scenario 2
Music group - with a translated field Chanson (fr) Song (en) Canzone (it)
and all other fields are suitable for all

avatar Bakual
Bakual - comment - 30 Nov 2016

We can always have something like #11040 and overcome the client side changes...

Potentially reloading all custom fields when the language changes doesn't sound like good UX to me. But yes, it is likely the only solution.

Music group - with a translated field Chanson (fr) Song (en) Canzone (it)

I had that in mind as well, but imho it is already possible using a translated label.
Except if the field value would be different as well on each language, but then how are you going to enter all three translations? The fields would all have to be visible in the form then so you can enter the various values.

I think that's likely something which should be discussed in a separate Issue.

avatar infograf768
infograf768 - comment - 30 Nov 2016

If the language is set to all on the article then all fields are loaded, which is right IMO.

I don't think so. Only fields set to ALL languages should display when an article is set to ALL. Fields set to ALL languages should also display for any article, whatever the language.

On a multilingual site chances are high the category is language specific as well.

High but not required. It depends on how you build the site

If one builds a site with items set to a specific language, then the hierarchy above that item (category, menu item) must OR be tagged to the same language OR to ALL languages.

One could do for example (but this is vicious and can lead to confusion...):

ROOT (set to ALL)
-Sub category 1 (set to ALL)
------> items set to ALL
------> items set to xx-XX
------> items set to yy-YY
--Sub-sub category1 (set to xx-XX)
-------> items set to ALL
-------> items set to xx-XX
--Sub-sub category2 (set to yy-YY)
-------> items set to ALL
------> items set to yy-YY
-Sub category 2 (set to yy-YY)
------> items set to yy-YY
------> items set to ALL

Etc.

In the case above, indeed, a field set to ALL and through the Sub category 1 (set to ALL) should display in all items down the hierarchy.
If the field is set to xx-XX and to be displayed in the same Sub category 1 (set to ALL), it should display only when the items are set xx-XX.

In any case, we may have here indeed, as @Bakual says, an unecessary complexity.

avatar laoneo
laoneo - comment - 30 Nov 2016

I would also not discus the language set up in a separate issue to stay focused here on the group implementation.

avatar ralain
ralain - comment - 30 Nov 2016

@bakual One final issue is that the move/copy radio buttons don't show up in the fields batch modal when selecting a group to move or copy to. Sent a PR with a possible fix to your branch.

avatar Bakual
Bakual - comment - 30 Nov 2016

Ah crap, didn't spot that. I'll see if I can add it somewhere without changing global JLayout files.

avatar Bakual
Bakual - comment - 1 Dec 2016

@ralain Should be visible now again. Thanks for testing, it's very appreciated!

avatar ralain
ralain - comment - 1 Dec 2016

My pleasure. I'll take another look tonight.

avatar infograf768
infograf768 - comment - 1 Dec 2016

I would also not discus the language set up in a separate issue to stay focused here on the group implementation.

Agree, but it has an impact though as batch functions use language as parameter and do not use Groups and /or Categories

avatar laoneo
laoneo - comment - 1 Dec 2016

When the user has the edit permission on the article but not the edit value permission on the field, I could change the value of the field.

avatar Bakual
Bakual - comment - 1 Dec 2016

When I tested the ACL this morning I used the editor group which by default has edit permissions on the articles. I was able to allow and restrict the ability to edit the value based on groups and fields as expected.

Did you try with a new field or with an existing one from before this PR? Because that could make a difference since the asset keys are different now (new is eg 'com_content.field.1`)

avatar ralain
ralain - comment - 1 Dec 2016

Confirming that copy/move works with the latest changes. However, we have a javascript error because the global script is still loaded and it assumes that at least one of the ids it's checking for is present.

screen shot 2016-12-01 at 18 46 43

avatar Bakual
Bakual - comment - 1 Dec 2016

I see. However that sounds like a bug in core to me. Technically it's loaded in the wrong JLayout (language instead of item). I guess one was lazy and didn't want to add it to the item, position and menu JLayouts but it's still wrong imho.
At least it should check if it is defined or not.

avatar Bakual
Bakual - comment - 1 Dec 2016

#13053 should fix it.

avatar ralain
ralain - comment - 1 Dec 2016

In that case, since I don't see any other issues (and I was unable to reproduce the problem @laoneo describes above,) I will mark this as successfully tested.

avatar ralain
ralain - comment - 1 Dec 2016

I have tested this item ✅ successfully on 6e15909


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

avatar ralain ralain - test_item - 1 Dec 2016 - Tested successfully
avatar tonypartridge
tonypartridge - comment - 2 Dec 2016

I have tested this item 🔴 unsuccessfully on 6e15909

I've tested this twice now on two fresh J3.7-dev clones with the JPatch Tester, all tests fail.

After applying patch I go to the JDatabase checker and apply fixes.

Then viewing Articles -> Fields we have no sidebar: https://www.dropbox.com/s/lttxhn376k75nqi/Screen%20Shot%202016-12-02%20at%2022.13.02.jpg?dl=0

URL was:
http://localhost/jpatch/administrator/index.php?option=com_fields&context=com_content.article

Then clicking Field Groups
We get:
https://www.dropbox.com/s/bwiepkda43793v6/Screen%20Shot%202016-12-02%20at%2022.15.11.jpg?dl=0

with the following url:
http://localhost/jpatch/administrator/index.php?option=com_fields&view=groups&extension=com_content

Also, the Joomla! Admin menu items to field groups is still linking to com_categories.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13019.
avatar tonypartridge tonypartridge - test_item - 2 Dec 2016 - Tested unsuccessfully
avatar Bakual
Bakual - comment - 2 Dec 2016

Hmm, that sounds like an issue with the patchtester then. It certainly works right here with the branch checked out.

avatar tonypartridge
tonypartridge - comment - 2 Dec 2016

I'll try it that way ;-)

avatar tonypartridge
tonypartridge - comment - 2 Dec 2016

Ok @Bakual yep you were right, I monitored the changes the JPatch tester applies and it's only 18 files.. only another 30 missing.

I've checked it out manually and it functions as intended now. A few points:

  1. Archive a field - Then filter by 'All' archived fields are not shown. Same applied for trashed fields.
  2. On saving we are being a bit general with the 'Item Saved' can we adjust this to 'Group Saved' and 'Field Saved' This is just a more precise way of notifying the users.
    This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13019.
avatar Bakual
Bakual - comment - 3 Dec 2016

I0'll have a look at those issues later. It's very likely not related to this PR but should be easy to fix.

avatar Bakual
Bakual - comment - 3 Dec 2016

@tonypartridge Fixed now.

avatar tonypartridge
tonypartridge - comment - 3 Dec 2016

Ok @bakual, I'll merge changes on Monday and run a test again :-) great work!

avatar tonypartridge
tonypartridge - comment - 3 Dec 2016

I have tested this item ✅ successfully on c4becd9

Great work @Bakual all working great now and messages are now spot on to me.


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

avatar tonypartridge tonypartridge - test_item - 3 Dec 2016 - Tested successfully
avatar ralain
ralain - comment - 4 Dec 2016

I have tested this item ✅ successfully on c4becd9


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

avatar ralain ralain - test_item - 4 Dec 2016 - Tested successfully
avatar jeckodevelopment jeckodevelopment - change - 4 Dec 2016
Status Pending Ready to Commit
avatar jeckodevelopment
jeckodevelopment - comment - 4 Dec 2016

RTC


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

avatar rdeutz rdeutz - change - 5 Dec 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-12-05 09:16:43
Closed_By rdeutz
avatar rdeutz rdeutz - close - 5 Dec 2016
avatar rdeutz rdeutz - merge - 5 Dec 2016
avatar rdeutz rdeutz - reference | d086a4a - 5 Dec 16
avatar rdeutz rdeutz - merge - 5 Dec 2016
avatar rdeutz rdeutz - close - 5 Dec 2016
avatar Bakual Bakual - head_ref_deleted - 5 Dec 2016
avatar coolcat-creations
coolcat-creations - comment - 24 Jan 2017

@Bakual Thank you for leading me to this PR. IMO i think such approach is better and more logical (and extendable later). Any thoughts?
image

avatar mbabker
mbabker - comment - 24 Jan 2017

Isn't there an open issue somewhere for this?

While I'm here, I'd say the way it is now is fine. It's also consistent with how categories are handled for extensions supporting those. Or are you saying you'd rather those all moved in favor of a categories item too?

Fields is integrated on a per-extension basis. For me that means I'd look for it with the extension.

avatar coolcat-creations
coolcat-creations - comment - 24 Jan 2017

For me it clutters the menu and doesn´t leave space for future improvements. Like that it would be isolated as an own Area to build custom forms for any extension that supports it...

avatar mbabker
mbabker - comment - 24 Jan 2017

You might be able to pull it off with your own custom admin menu. But the way core handles things, it's too complex to do that by default.

I honestly see the number of top level menus as clutter and adding one more makes it worse.

avatar Bakual
Bakual - comment - 24 Jan 2017

This should be discussed in an own issue, not in a merged PR. I just linked it because it was discussed here already before the current state got merged.

adding one more makes it worse.

The issue arose because there is another PR now which may raise those field links in the sidebar from currently two to three. It's not one single link like for categories.

Add a Comment

Login with GitHub to post a comment