Problem/Motivation

The configuration synchronization UI needs work. Issues include:

  • It is just a list of that can get longer and longer.
  • It is sorted by the order the import will occur.
  • Special processes like module and theme installation and uninstallations are hidden in the core.extension diff
  • Special processes like deleting field data before uninstalling modules are drupal_set_messages
  • There is no confirmation form and the operation is certainly desctructive
  • There is no clear indication when certain operations (field purge on module uninstall) may take a very long time.

Proposed resolution

  • Reorder the tabs for the Config UI as follows:
    New tab organisation

Remaining tasks

  • Discuss
  • Design
  • Implement

User interface changes

As listed in Proposed Resolution

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because current UI works but is not as usable as it could be
Issue priority Major because having a usable UI is important for D8
Prioritized changes The main goal of this issue is usability.
Disruption Disruptive for contrib or custom if altering the config sync forms. The benefits outweigh this theoretical disruption.
Files: 
CommentFileSizeAuthor
#129 reorder_tabs_in-2247291-129.patch3.06 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,901 pass(es). View
#122 reorder_tabs_in-2247291-122.patch3.03 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,901 pass(es). View
#109 9 - Welcome_to_drupal_8_0_x_WITH_CONFIG_CHANGE___drupal_8_0_x_WITH_CONFIG_CHANGE.png67.52 KBEli-T
#109 8- Synchronize___drupal_8_0_x_WITH_CONFIG_CHANGE.png35.05 KBEli-T
#109 7- Confirm_configuration_import___drupal_8_0_x.png20.54 KBEli-T
#109 6 - review and import all.png59.35 KBEli-T
#109 5 select and upload config.png57.73 KBEli-T
#109 4 - export tabs.png32.17 KBEli-T
#109 3 - import tabs.png42.99 KBEli-T
#109 2 - main tabs.png34.26 KBEli-T
#109 1 - admin-config.png57.6 KBEli-T
#98 interdiff-91-98.txt3.83 KBEli-T
#98 configuration-2247291-98.patch35.38 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,816 pass(es). View
#95 interdiff-91-95.txt3.62 KBEli-T
#95 configuration-2247291-95.patch35.38 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,041 pass(es). View
#93 interdiff-81-91.txt16.94 KBEli-T
#91 configuration-2247291-91.patch33.04 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,939 pass(es). View
#88 configuration-2247291-88.patch33.26 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,636 pass(es). View
#81 configuration-2247291-81.patch36.02 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,498 pass(es), 2 fail(s), and 0 exception(s). View
#78 configuration-2247291-78.patch48.43 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 97,650 pass(es), 297 fail(s), and 2 exception(s). View
#76 configuration-2247291-76.patch46.86 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch configuration-2247291-76.patch. Unable to apply patch. See the log in the details link for more information. View
#71 interdiff-2247291-68-71.txt4 KBEli-T
#71 configuration-2247291-71.patch70.31 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,294 pass(es). View
#68 configuration-2247291-68.patch70.36 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,271 pass(es), 7 fail(s), and 0 exception(s). View
#65 configuration-2247291-65.patch68.01 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,331 pass(es). View
#59 configuration-2247291-59.patch55.63 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,221 pass(es), 67 fail(s), and 1 exception(s). View
#56 configuration-2247291-56.patch59.43 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch configuration-2247291-56.patch. Unable to apply patch. See the log in the details link for more information. View
#52 configuration-2247291-52.patch27.88 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,275 pass(es), 17 fail(s), and 0 exception(s). View
#51 view changes text change required.png66.6 KBEli-T
#42 configuration-2247291-42.patch23.06 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,137 pass(es), 5 fail(s), and 0 exception(s). View
#41 configuration-2247291-41.patch22.31 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,124 pass(es), 10 fail(s), and 0 exception(s). View
#39 Import___AWESOME_SITEZ.png120.42 KBEli-T
#39 configuration-2247291-39.patch22.26 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,129 pass(es), 6 fail(s), and 0 exception(s). View
#38 6 if you upload but dont import and revisit the page later.png119.83 KBEli-T
#38 5. confirmation.png116.39 KBEli-T
#38 4 review changes and import.png182.78 KBEli-T
#38 3.select file and upload.png99.9 KBEli-T
#38 2.initial full import screen.png104.5 KBEli-T
#38 1.Config option.png96.32 KBEli-T
#36 configuration-2247291-36.patch21.76 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,020 pass(es), 10 fail(s), and 0 exception(s). View
#18 configuration-2247291-18.patch1.71 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,429 pass(es), 5 fail(s), and 0 exception(s). View
#16 configuration-2247291-16.patch1.69 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,431 pass(es), 5 fail(s), and 0 exception(s). View
#7 configuration management pages.png186.94 KByoroy

Comments

xjm’s picture

Issue tags: +Usability

Yes, it does. :) There are probably existing issues related to this... somewhere...

xjm’s picture

Issue summary: View changes

Also adding @webchick's feedback re: warning the user about long-running batches.

Bojhan’s picture

I wouldn't mind helping out with this but I need some more guidance.

moshe weitzman’s picture

Maybe we shouold post some screenshots to the UX folks can do their thing.

yoroy’s picture

Actually, I've been looking at this UI this week. Incomplete first notes:

On first visit, after creating a new content type:

- Title doesn't match the link I clicked. I click 'Configuration management' and end up on a page titled 'Synchronize'.
- The first thing I see is a warning that my current configuration has changed.
- Not clear what the difference is between single and full import/export
- Apparently, synchronisation is considered the most important use case?
- "Import configuration that is placed in your staging directory. All changes, deletions, renames, and additions are listed below." Well, the only changes listed on first visit are listed in the message above. Missing a hint as to where the staging directory might be.

I was thinking I could use the config manager to keep track of a content type I want to use when testing out D8, but it won't let me import an earlier export because it considered my site a new site (probably because I recreated the database in between?)

I uploaded the tar.gz of a complete export but got the error message as mentioned above.

- The lack of pointing out where the staging directory lives becomes serious now because apparently there's something there that won't work and I don't get a hint on how to remove it. Asking in IRC helps but that's not a solution :)

I have only worked with the import/export screens superficially:

- Single import/export has duplicate entries for 'Field' in the select list
- Generally lacking context, no hints given as to what you might be able to achieve on these screens
- The contents of the 'Advanced' fieldset might need a better description as to *why* you'd want to use this
- Full import/export screens are both very light on functionality, a single upload and a single download button

yoroy’s picture

Some user stories or scenarios for what people will want to do here would be helpful. When do I sync? When import, when export?

yoroy’s picture

I have a local setup of two sites now to experiment with. There's issues on each individual page (esp. the 'synchronize' screen can be confusing), but for now I wonder about the information architecture of the screens in this section. Why the main split between single and full? From playing a bit with importing and exporting changes, I find I first consider wether I want to import or export, and after that decide between full or single:

jhodgdon’s picture

+1000 on that last idea.

What do we need to move forward on making this system at all usable? It's really kind of terrible right now.

yoroy’s picture

jhodgdon’s picture

Are changes to the UI going to happen soon? Because right now #1831798: Update hook_help() for config manager module is postponed on this issue. That is about making help blocks and the overall help page for the Config Manager module -- which is desperately needed, there's practically nothing there now. Should we go ahead there and then revise it if/when this is done, or wait for this issue to happen?

mtift’s picture

I really like the suggestion in #2247291-7: Reorder tabs in configuration UI, too

jhodgdon’s picture

So regarding the design in #7, ... on the related/child issue #2388867: Notifying user of config changes when config has never been synched makes no sense it has been brought up that the current Synchronize page has two purposes. Let's see if I can get this right... maybe more than two actually:

a) Notify you that config has changed since the last snapshot. [As noted on that related issue, this notification is very misleading after an initial site install, since it tells you a bunch of stuff has changed immediately after the install, just due to the stuff you configure during install or that is configured for you by the Standard profile.]

b) Bulk synchronize from staging to active, which has a side effect of making a new snapshot.

Also the bulk Import page, just to clarify: what it actually does is to un-archive a tgz file into the staging directory, and then drop you onto the Synchronize page so you can actually make the config you imported live. So in essence, the bulk "import" page doesn't really actually do anything to your config, unlike the single import page. And there's no help on the page to explain this... that's a separate issue being explored on #2203779: Improve wording of the configuration import form, which I'll mark as Related. But it may impact the idea of the new navigation, since really bulk Import is tied to Synchronize?

dawehner’s picture

I have to agree that the current grouping is super annoying ... You really often want a single export, but therefore you need to click twice.
With the new proposal in #8 you would save a single click here.

jhodgdon’s picture

So, another thought on the #7 design proposal.

I guess my basic question is whether most users will even use the Status/Synchronize page. It really only does something if you put config into the Active staging area, which is really kind of a subset of the bulk Import functionality. So maybe we should think again about the organization of pages, and put something else at the forefront?

Eli-T’s picture

Assigned: Unassigned » Eli-T
Eli-T’s picture

FileSize
1.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,431 pass(es), 5 fail(s), and 0 exception(s). View

Simple patch that implements the renaming and moving of the tabs as proposed in #7

Eli-T’s picture

Note also if the permissions are incorrect on the files in the config staging directory and the web server cannot read them, the error message

The staged configuration cannot be imported, because it originates from a different site than this site. You can only synchronize configuration between cloned instances of this site.

is displayed on the Synchronisation (or status) page. This is potentially confusing and could be expanded to cover the possibility that the webserver just can't read the config files.

Eli-T’s picture

FileSize
1.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,429 pass(es), 5 fail(s), and 0 exception(s). View

Set page title to Configuration management to be consistent with the link that takes you here from Configuration, as pointed out in #5 (includes changes from patch in #16)

alexpott’s picture

Status: Active » Needs review

The last submitted patch, 16: configuration-2247291-16.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: configuration-2247291-18.patch, failed testing.

Eli-T’s picture

There are further issues with the solution as proposed and I think some further restructuring could add clarity for users.

Issues:

  • When performing a full import, the actions on the import tab do not complete the import. They merely upload the new configuration. Therefore a case could be made to rename the import tab to "Upload"
  • When performing a single import, the actions on the import tab *do* complete the import. Therefore a case could be made to not rename the import tab
  • The information on the status page is only relevant when performing exports

Therefore I propose that the Status (or Synchronize) tab actually be removed. The functionality of that page should be moved to the Import tab, which will now contain all the functionality to import both full and single configs, removing any need to rename the tabs or split Import Single and Upload Full in to their own tabs.

Eli-T’s picture

NB tests expected to fail at this point.

ivanstegic’s picture

Hi everyone, we (@Les Lim, @lexfunk, @JackProbst, @ColemanRollins, @jascote, @tiffdmoore and I) just spent about 2 hours talking through this issue, and #2203779: Improve wording of the configuration import form here in the office.

We all agreed with the idea that there needs to be some reorganization of the tabs. We applied the patch in #18, worked just fine, and we all agree that the best option moving forward is to remove the Status (Synchronize) tab, as mentioned in #22. Thoughts:

  • A person could potentially upload files to the staging area without using the UI, so there still needs to be a place to see a diff.
  • Retain "Import" and "Export" tabs, with "Full" and "Single" under each.
  • Default to "Import > Full" when accessing "Configuration > Development > Configuration management" that shows a current diff, and includes a file package upload fieldset, above or below the "Status diff". If there is no diff, the fieldset is still there so you that you upload a package using the UI, if you want to.
  • "Import > Single", "Export > Full", "Export > Single" continue as is.
  • Change the URLs, they should now be "admin/config/development/configuration/import/full", "admin/config/development/configuration/import/single", "admin/config/development/configuration/export/full" and "admin/config/development/configuration/export/single" to reflect the new tabs.
  • Functionality is broken when JS is switched off, there's probably a bigger issue here we are not aware of?
  • The text area under "Export > Single" could probably be set to read-only, and there are probably some other tweaks we'd want to make to the functionality of this form. I'll create a separate issue for that.
  • There should be a confirmation screen after submitting the actual import of the configuration, just in case. Maybe this is another issue?
jhodgdon’s picture

This all sounds good. I'm slightly confused about where you'd go to see a diff of what you uploaded, though?

ivanstegic’s picture

I think that would be under "Import > Full", wouldn't it?

Eli-T’s picture

Yes, the diff of the upload is only relevant when performing a Full Import.

jhodgdon’s picture

Let me rephrase. I thought there was a difference between the current Synchronize page and the Full Import page? I have never really been sure, but I thought so, since there are two pages now and they are definitely not the same. So it seems like we need to not lose this page entirely?

alexpott’s picture

@jhodgdon the full import just lets you upload a file - which will let you do the sync - the proposal I think is to merge the two. Makes lots of sense to me.

Eli-T’s picture

There is definitely a difference between those two pages. I am proposing moving the operations you would carry out on the Synchronize page on to the Full Import page. Nothing would be lost, it will just be grouped together to make more sense.

The current flow for a full import is:

  1. Navigate to full import page
  2. Select config.tar.gz to upload from the local file system
  3. Press Upload
  4. User is automatically redirected to the synchronize/status page
  5. Review differences in configuration
  6. Presses Import all

So at the point when the user is redirected from the Import tab, they still haven't completed an import. They've merely completed an upload. Then the import is actually completed on a tab called Synchronisation.

So the Import doesn't import, it merely uploads.

If we change the flow to

  1. Navigate to full import page
  2. Select config.tar.gz to upload from the local file system
  3. Press Upload
  4. Review differences in configuration whilst still on the Import tab
  5. Presses Import all whilst still on the Import tab

the import will be completed on a tab called Import. The Synchronisation page becomes redundant. The interface becomes simpler and is more meaningfully named.

jhodgdon’s picture

Excellent. Thanks very much for the explanation! I had been confused about how Synch and Import were related; much less so now. And these changes make tons of sense to me.

I also think that landing on either Import or Export when you click through from Admin -> Configuration makes a lot more sense than landing on an incomprehensible Synchronize page. I use Export Single and Import Single a *lot* (while developing modules, or to share a View between sites or something like that), but I have no use for Synchronize yet (I think it is only for maintaining Dev -> Staging -> Live workflows).

One question though. Can advanced users bypass the Full Import screen by putting config files somewhere themselves? If so, is there a way for them to get to the "review differences and import all" screen, without first going through an Upload? And what if someone has used Upload, then went to do something else, can they get back to the "review differences and import all" screen?

I guess the workflow would be:
If you go to Import Full and there's already something in the staging area, show the "review differences and import" screen. If not, show the Upload screen. ??? maybe? And would there be a way to discard staged changes?

Eli-T’s picture

I think it would be *slightly* different; show the upload controls whether there's something in the staging area or not. If there is something in the staging area, *also* show the review differences and import controls.

jhodgdon’s picture

That sounds like a good idea to me. If there's something already there, presumably/hopefully it will warn you before overwriting anyway. I would hope.

jhodgdon’s picture

We need to get this done... Eil-T are you still working on it?

Eli-T’s picture

I am planning to work on this tomorrow evening.

Eli-T’s picture

Status: Needs work » Needs review
FileSize
21.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,020 pass(es), 10 fail(s), and 0 exception(s). View

This patch is functional and merges the Synch tab functionality in to the Full Import

Some clean-up still required and test adjustment but should illustrate whether this is the right approach.

Status: Needs review » Needs work

The last submitted patch, 36: configuration-2247291-36.patch, failed testing.

Eli-T’s picture

With the patch above, flow for a full import is

1. Select config management

Select config management

2. Takes you to Full Import by default

Full import

3. Select file and upload as usual

Select file and upload

4. Review changes and import from the same screen

Review changes on the same screen

5. Receive confirmation that new config has been imported

Confirmation

6. If you don't import at this point, if you revisit the Full Import page in the future you still have the option to review and import.

Review a previously uploaded but unimported config file

Eli-T’s picture

Issue summary: View changes
FileSize
22.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,129 pass(es), 6 fail(s), and 0 exception(s). View
120.42 KB

New patch replacing the text which refers to redirecting to the import form with "'Uploaded configuration changes can be reviewed below before they are imported.'"

Also add the warning text "This will replace your previously uploaded configuration." if and only if config has previously been uploaded, is valid to import and differs from the site's current config.

jhodgdon’s picture

Thanks for all these screen shots! Looking good!

I had a few thoughts/suggestions about the UI text:
- We definitely need to lose the text at the top that says "Use the upload button below". That falls into the department of redundancy department.
- "Select your configuration export file" seems a bit weird to me, since you are importing. How about "Configuration archive" as the field title? Or whatever wording is used on the Full Export form to describe the file that is being exported.
- Some hook_help() text at the top of the Import page, explaining where you might get a configuration archive and what it is, would be helpful. I think it would also need to explain about the UUIDs that have to match in order for this import to even be allowed.
- I thought the upload field had a description saying what file types are supported, where did this go? Or maybe that was on a different issue, but I remember discussing it could be .tar.gz, .tgz, or one other format? Hm, seems like it is on a different issue. Oh yes, it's on #2203779: Improve wording of the configuration import form... why do we have two issues for this? Anyway the UI text is being discussed there; updates there need to make it onto this issue.
- "This will replace ... " warning should be in a confirm form rather than being on this page where people might not notice it. That is the standard way to warn users that something they are doing is destructive and/or permanent. I think the text should say "This will replace the configuration that was previously uploaded for import. Proceed?" or something like that.
- Hopefully the "Import all" button also has a confirm form, telling the user that their site configuration will be changed?
- "Uploaded configuration changes can be reviewed below before they are imported"... I am not sure I like the word "below" there, because they aren't below when you first get to this page. I think I would just remove"below".

Eli-T’s picture

FileSize
22.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,124 pass(es), 10 fail(s), and 0 exception(s). View

This patch copies the string changes from the patch at https://www.drupal.org/node/2203779#comment-10047104 with the exception of the implementation of hook_help() on the config sync page, which no longer exists.

Eli-T’s picture

FileSize
23.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,137 pass(es), 5 fail(s), and 0 exception(s). View

We definitely need to lose the text at the top that says "Use the upload button below". That falls into the department of redundancy department.

Removed

- "Select your configuration export file" seems a bit weird to me, since you are importing. How about "Configuration archive" as the field title? Or whatever wording is used on the Full Export form to describe the file that is being exported.

Superceded by #2203779

- Some hook_help() text at the top of the Import page, explaining where you might get a configuration archive and what it is, would be helpful. I think it would also need to explain about the UUIDs that have to match in order for this import to even be allowed.

Implemented hook_help() with dummy text until we have words for here.

- I thought the upload field had a description saying what file types are supported, where did this go? Or maybe that was on a different issue, but I remember discussing it could be .tar.gz, .tgz, or one other format? Hm, seems like it is on a different issue. Oh yes, it's on #2203779: Improve wording of the configuration import form... why do we have two issues for this? Anyway the UI text is being discussed there; updates there need to make it onto this issue.

Thanks for pointing the issue, I missed it in the related list. Copying changes from there as we go now.

- "This will replace ... " warning should be in a confirm form rather than being on this page where people might not notice it. That is the standard way to warn users that something they are doing is destructive and/or permanent. I think the text should say "This will replace the configuration that was previously uploaded for import. Proceed?" or something like that.

- Hopefully the "Import all" button also has a confirm form, telling the user that their site configuration will be changed?

Will implement these two confirmation forms next. Do you think we should display the confirmation dialog for upload if there is a) no previously uploaded config, and if b) there is uploaded config but that matches the current active config? I think a case can be made that in those two scenarios it is not a destructive operation.

- "Uploaded configuration changes can be reviewed below before they are imported"... I am not sure I like the word "below" there, because they aren't below when you first get to this page. I think I would just remove"below".

Superceded by #2203779

pjonckiere’s picture

Status: Needs work » Needs review

Triggering test bot.

Implemented hook_help() with dummy text until we have words for here.

The help text will be handled in #1831798: Update hook_help() for config manager module .

Eli-T’s picture

Tests anticipated to fail :)

The last submitted patch, 39: configuration-2247291-39.patch, failed testing.

The last submitted patch, 41: configuration-2247291-41.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 42: configuration-2247291-42.patch, failed testing.

jhodgdon’s picture

Regarding the confirm forms, I agree that if there is no previous uploaded config, there is no need for the warning message. But I don't think you will be able to check whether the new config the person is trying to upload is the same as the previous upload? The confirm form should come before the upload takes place?

Eli-T’s picture

But I don't think you will be able to check whether the new config the person is trying to upload is the same as the previous upload?

We don't have to. I just want to check if the previous upload is different to the active config.

jhodgdon’s picture

Oh, I misread that. Sounds like a good idea. :)

Eli-T’s picture

Just realised we need to update this string too:

Eli-T’s picture

FileSize
27.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,275 pass(es), 17 fail(s), and 0 exception(s). View

Confirmation form for import step implemented, and rerolled after commit of #2203779.

Eli-T’s picture

Status: Needs work » Needs review

Putting to review to make sure nothing outside of config is broken by these changes.

Status: Needs review » Needs work

The last submitted patch, 52: configuration-2247291-52.patch, failed testing.

Eli-T’s picture

Another thing to consider - there were separate permissions for 'synchronize configuration' and 'import configuration'. Now that they are grouped on to the same form, it feels like we should consolidate to just 'import configuration'.

Eli-T’s picture

Status: Needs work » Needs review
FileSize
59.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch configuration-2247291-56.patch. Unable to apply patch. See the log in the details link for more information. View

Consolidated 'synchronize configuration' and 'import configuration' permissions.
Fix tests.
Change Back to Synchronize Configuration to Back to Full Import.
Remove ConfigSync form and redundant routes.
Add title and confirm button text to import confirmation form.

Still need to implement the new hook_help changes in #1831798 but will wait for that to be complete and then adapt. Nevertheless, probably a good idea for someone else to cast their eyes over this.

I *haven't* implemented a confirm dialog for upload; after consideration I don't think it's particularly destructive as it will ony overwrite staged config.

Status: Needs review » Needs work

The last submitted patch, 56: configuration-2247291-56.patch, failed testing.

Eli-T’s picture

Issue tags: +Needs reroll
Eli-T’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
55.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,221 pass(es), 67 fail(s), and 1 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 59: configuration-2247291-59.patch, failed testing.

jhodgdon’s picture

Agreed on the "no confirm for upload" idea.

cilefen’s picture

I tried some renaming of page titles in a related issue #2513568-3: Relabel "Configuration Management". Those were out-of-scope in the other issue. The renames were:

Synchronize => Synchronize site configuration
Export => Export full site configuration
Import => Import full site configuration
Single import => Import single configuration
Single export => Export single configuration

Eli-T’s picture

Bojhan’s picture

I am just reviewing this, below is a whole list of ideas and t

Import:

  • We show an upload field above the table. However this is the "action" relating to the listing. In this case it would be more consistent UI pattern (e.g. menu's, content types, etc.) to have a "Import configuration" button that takes you to a upload form. Rather than having this on top of the table. When users have only 1 button to press apply to, it usually works better.
  • Label on the button for importing should say "Import changes" or "Import configuration".
  • Can we just say "file" and not "archive" seems overly complex, for what we are trying to signal here
  • Before the upload page, you could potentially make the user decide between uploading a text or uploading per category (just like we do with node/add).

Export:

  • The export page itself lists nothing? I thought this would show me what I have changed from the default/last configuration
  • The labeling for the secondary configuration could be optimised, the word single has no semantic meaning.
    • All configuration
    • Per configuration type
  • The single page is riddled with labels that have little meaning and/or misalign with our text standards. Please have a look at https://www.drupal.org/node/604342, and lets have a sensible label for the textarea.

I don't really understand why in core we want to also support single export/import. Isn't that something that is better served in contrib? I can't imagine that these large dropdowns is really what you want.

Eli-T’s picture

Status: Needs work » Needs review
FileSize
68.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,331 pass(es). View

This patch should fix the remaining tests with the current approach before https://www.drupal.org/node/2247291#comment-10069630 was written.

We need further input to decide whether all the changes suggested there need to happen - if so I probably won't be able to start on them for at least a week.

NB even though put to needs review, there is still a piece of placeholder text in hook_help that is awaiting https://www.drupal.org/node/1831798 to be completed before it can be adapted to the new organisation of forms so this shouldn't be committed yet.

jhodgdon’s picture

One or two responses to #64:

a)

"The export page itself lists nothing? I thought this would show me what I have changed from the default/last configuration"

Core doesn't currently have any easy way to tell what you have changed from defaults. I managed to get this working in my contrib module https://www.drupal.org/project/config_update but it was not exactly easy, and while I believe I suggested this for Core it was deemed 8.1 or later material, or else contrib... hence the contrib module.

b) Just to clarify: you cannot import or export configuration "by category". You can only import a single item. The "by category" lists mentioned in #64 are really the entity types of the config entities (plus "simple" for the config that isn't entities). You cannot export an entire entity type at once. On export, the type list is just to help you navigate through the hundreds of items you could export; on import, you have to know what type you have in order to validate the import.

So to me, "single" means "one config item", and if the wording were changed to "by category" or "by type", it would imply that I could import a whole subset of my config. It definitely does not do that.

c)

I don't really understand why in core we want to also support single export/import. Isn't that something that is better served in contrib?

If we want to remove this to contrib, the above mentioned config contrib module would be a good home for it. To me that is essential functionality, where "me" means "a contrib module developer", because that is the best way to export the config I want to put into the config/install directory (like default views). But anyone who is not a contrib module developer probably doesn't need this (although this is the only way to export/import Views, which was a 7.x Views functionality I personally used quite a bit in my Site Builder For Hire role). ???

Anyway, I would be fine with removing it to my https://www.drupal.org/project/config_update module (which might then be better served by a different name, but oh well).

Eli-T’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
Eli-T’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
70.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,271 pass(es), 7 fail(s), and 0 exception(s). View

Reroll against latest 8.0.x

Status: Needs review » Needs work

The last submitted patch, 68: configuration-2247291-68.patch, failed testing.

Eli-T’s picture

Test fails because #1831798: Update hook_help() for config manager module refers to url('config.sync') on /admin/help/config, which has been removed in this patch.

Eli-T’s picture

Status: Needs work » Needs review
FileSize
70.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,294 pass(es). View
4 KB

Removes references to config.sync in config_help().
Removes references to default.services.yml and default.settings.php included in the patch in #2247291-68: Reorder tabs in configuration UI

Eli-T’s picture

NB this isn't fit for merging right now as there is still a lorem ipsem string in config_help() for config.import_full that needs revisiting, but it's still a good time to review given the amount of work carried out so far.

alexpott’s picture

I've had a look at the re-arrangement. Moving the upload form to the import all screen is difficult because it is making an assumption about the workflow. Also once you have config in staging to import having a very prominent upload button is confusing. We need to support at least two different workflows here - one where a user is using the config tarballs and one where configuration is moved to staging in another way.

Wrt to #64 - I don;t think we can remove the single/import export. Let's file a separate issue to improve how these pages work. This issue is about the organisation of the different tasks possible within "Configuration synchronization" and making sure that we support the different full import workflows.

jhodgdon’s picture

Status: Needs review » Needs work

Sounds like this is Needs Work for #73?

Eli-T’s picture

Yep, thanks for the feedback @alexpott. I'll revert the combination of the import and synchronization tabs, but keep the tab reorganisation and the import confirmation form.

Eli-T’s picture

FileSize
46.86 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch configuration-2247291-76.patch. Unable to apply patch. See the log in the details link for more information. View

Tabs rearranged. Sync tab restored. Confirm form moved to sync tab.

Eli-T’s picture

Issue tags: +Needs reroll
Eli-T’s picture

Status: Needs work » Needs review
FileSize
48.43 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 97,650 pass(es), 297 fail(s), and 2 exception(s). View

Reroll against latest 8.0.x

Eli-T’s picture

Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 78: configuration-2247291-78.patch, failed testing.

Eli-T’s picture

Status: Needs work » Needs review
FileSize
36.02 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,498 pass(es), 2 fail(s), and 0 exception(s). View

Many many test fixes (but not all)

Status: Needs review » Needs work

The last submitted patch, 81: configuration-2247291-81.patch, failed testing.

jhodgdon’s picture

Looks like you got all but 2 of them!

Eli-T’s picture

Assigned: Eli-T » Unassigned

I've been looking at this last night and today and can't fathom why the changes made are causing these last two tests to fail in this way. I have no more time to look at this today and possibly not for another few days so unassigning in case anyone else wants to pick up.

tim.plunkett’s picture

  1. +++ b/core/modules/config/src/Form/ConfigSync.php
    @@ -119,7 +127,8 @@ class ConfigSync extends FormBase {
    -   * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher
    +   * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface
    +   * $event_dispatcher
    

    This needs to be on one line

  2. +++ b/sites/default/default.services.yml
    @@ -51,7 +51,7 @@ parameters:
    -    # https://www.drupal.org/node/1906392.
    +    # http://drupal.org/node/1906392.
    

    Please revert all of these

There are a *lot* of out of scope changes here. Please try to revert all of those.

Also it seems it's failing because it's hitting the confirm form now instead of the previous action of just syncing.

Eli-T’s picture

With the @param changes, phpcs was complaining because the lines were > 80 chars, but didn't complain if they were split over two lines. What's the correct way to fix that? Or is my phpcs standards definition out of date?

Also it seems it's failing because it's hitting the confirm form now instead of the previous action of just syncing.

I think the fail at line 96 occurs before that confirm form should make a difference?

tim.plunkett’s picture

I don't use phpcs, and it often contradicts our coding standards.

Eli-T’s picture

Status: Needs work » Needs review
FileSize
33.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,636 pass(es). View

Fix the source of the remaining test failures. Put to needs review to prove tests, but note out of scope changes in #85 still need addressing.

Eli-T’s picture

Status: Needs review » Needs work

Put back to needs work to address #85

Eli-T’s picture

Assigned: Unassigned » Eli-T
Eli-T’s picture

Status: Needs work » Needs review
FileSize
33.04 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,939 pass(es). View

Wrap Import in t() when defining tests.
Remove unused properties from classes where functionality has moved from Config Sync to Confirm Form.
Fix permission comments in tests.
Fix coding standards.
Add missing @param declarations.

Eli-T’s picture

Issue summary: View changes
Eli-T’s picture

FileSize
16.94 KB

Interdiff to the last reviewed version.

alexpott’s picture

Status: Needs review » Needs work

Tested import and export through the UI and everything is working nicely.

I think we should open a followup to discuss the single import / export UI since that came up in this issue. I also think we should open a followup to change the message on synchronisation page if the staging folder is empty. Since it should be different when there is nothing to import because it matches.

  1. +++ b/core/modules/config/src/Form/ConfigSync.php
    @@ -74,39 +52,18 @@ class ConfigSync extends FormBase {
    +   * The key value storer.
    

    storer => store

  2. +++ b/core/modules/config/src/Form/ConfigSync.php
    @@ -319,46 +256,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -          'finished' => array(get_class($this), 'finishBatch'),
    ...
    -          $batch['operations'][] = array(array(get_class($this), 'processBatch'), array($config_importer, $sync_step));
    

    We can now remove ConfigSyncForm::processBatch() and ConfigSyncForm::finishBatch() as they are no longer needed.

  3. +++ b/core/modules/config/src/Form/ConfigSyncConfirmForm.php
    @@ -0,0 +1,279 @@
    +  /**
    +   * Finish batch.
    +   *
    +   * This function is a static function to avoid serializing the ConfigSync
    +   * object unnecessarily.
    +   */
    +  public static function finishBatch($success, $results, $operations) {
    

    I know this is a copy and paste job but let's document that this is a batch callback. Let's copy and amend the docs for _node_access_rebuild_batch_finished

Eli-T’s picture

Status: Needs work » Needs review
FileSize
35.38 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,041 pass(es). View
3.62 KB

Comments in #94 addressed.

Eli-T’s picture

Eli-T’s picture

FileSize
35.38 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,816 pass(es). View
3.83 KB

Remove redundant use Drupal\Core\Config\ConfigImporter; from ConfigSync.php; not required now redundant ConfigSync::processBatch() and ConfigSync::finishBatch() have been removed.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for addressing everything from #94. I think this is good to go.

The last submitted patch, 76: configuration-2247291-76.patch, failed testing.

Eli-T’s picture

Go home drupal.org, you are drunk.

#100 by d.o says

The last submitted patch, 76: configuration-2247291-76.patch, failed testing.

but the last submitted patch was configuration-2247291-98.patch, and that passed testing.

Eli-T’s picture

Assigned: Eli-T » Unassigned
alexpott’s picture

Issue summary: View changes

Added beta evaluation.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

alexpott’s picture

effulgentsia’s picture

Assigned: Unassigned » webchick

Since this is primarily about changing a UI, assigning to webchick for final review/commit.

Eli-T’s picture

Title: Configuration synchronization UI needs a usability review » Reorder tabs in configuration UI and add confirm dialog before full import

Retitle after conversation with @berdir in IRC

Eli-T’s picture

Final screenshots to clarify

Select Configuration Synchronisation from admin/config

Tabs structure now based on what you are doing first rather than how much you are doing

Import tab has subtabs Full and Single

As does export

All these processes work exactly as they did before with the exception of Synchronize. That now has an added confirmation step.

So assuming you already have a full config set you wish to import, select and upload it from /admin/config/development/configuration/full/import:

You are redirected to the Sync page as before, but this time after pressing Import All...

..you are presented with a confirmation form before the destructive operation of importing the configuration is carried out

Press import and the new config is imported, before you are redirected to the sync screen and confirmation

and we can see the effects of the config import

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

Wow, great, thanks for the screenshots!

So... these screenshots really highlight that we are still using the word "import" in this UI for two or maybe even three different things, and Synchronize is only the page title and everywhere else in help/UI it says "Import. Here's what I mean:

a) On the Single Import page, Import means "Paste in some config YAML, and we'll put it directly into the active config".

b) On the Full Import page, Import in the help means "Upload a zip file, and we'll unzip it into your config staging directory" (the button is called Upload, which is good at least). This doesn't really seem all that related to the operation of Import on the Single import age. It really belongs grouped with the operation that is on Synchronize, I think, because it doesn't actually do anything to your site unless you also go to Synchronize. Really it's an Upload operation as part of Synchronize, right?

c) On the Synchronize page, which confusingly has no help or UI text aside from the page title that say "synchronize" on them (it all talks about importing), clicking the Import button means "Take the changes currently in your config staging directory, and put them into the active config". I have no idea why it is called Synchronize at all, because the help at the top and the button call it Import, and it's really the second step in the Full Import process (or I guess you could have uploaded the file separately).

So. This still seems pretty confusing. Is it out of scope to ask that we clear up the terminology and help so that they make more sense?

I think it would really make more sense to have:

Full import
- Synchronize tab that would be the current page that does the importing into active
- Upload tab that would be the current Import / Full page that uploads and unzips config into staging
Single import
Full export
Single export

(or those last two tabs could be as they are now, Full and Single tabs under Export)... Or maybe

Import
- Full import [what is currently called Synchronize]
- Full upload
- Single
Export
- Full
- Single

I don't know... but if we're sticking with a page called Synchronize, we should also update the page help so it uses that terminology, and ... that Full Import page should be called Upload and definitely belongs grouped with the Synchronize page.

Eli-T’s picture

Status: Needs review » Reviewed & tested by the community

Returning to RTBC following discussion with @jhodgdon and @alexpott. Whilst there are further improvements that can be made in this area, this patch improves the UI and does not exacerbate any existing problems.

jhodgdon’s picture

If we commit this as-is, then I would suggest a followup where we:

a) Consolidate this further into two main tabs, Import and Export, with the following sub-tab structure:
Import
- Synchronize (with all language/buttons on this page changed from saying Import to Synchronize)
- Upload full (making sure it doesn't use "import" terminology to mean "upload and unzip a file", and making sure it mentions that after upload you need to synchronize)
- Single
Export
- Full
- Single

b) Update all the hook_help() [both module topic and page-level help] so it matches the UI and makes the terminology of Import, Synchronize, and Upload clear.

Eli-T’s picture

Status: Reviewed & tested by the community » Needs work

Having slept on this, I don't think the disruption of changing the tab structure twice in two successive issues is worth it. So am happy to rework this in this issue if consensus can be agreed by all the relevant parties (webchick, yoroy, jhodgdon, alexpott) on the way forward.

yoroy’s picture

I think jhodgdons concerns are very valid. Especially about clearly differentiating between upload and actual syncing.
Not sure on wether that should be a follow up or not. I don't mind incremental progress, at 100+ comments in it may be better as a follow up?

tsvenson’s picture

This issue got to my attention from the #drupal-usability IRC channel.

Have read through the summary and comments from the screenshots i #109 and forward.

I think the choice of "Full" and "Single" can be cause to some confusion as a full synchronization is also a single (one time) synchronization workflow.

Change of words:

  • Full -> Complete
  • Single -> Individual items

This should also help to improve readability for the information text as the distinction between "complete" and "individual" is clearer than "full" and "single" when comparing "A full synchronization" with "A single synchronization".

I'd be happy to do a screenshare review where someone with the backend knowledge can demonstrate and walk through the process involved for a user performing these actions.

jhodgdon’s picture

So... Another idea. The tab structure now, as well as the latest patch, is rather cumbersome if you navigate using the existing menu system -- requires a lot of clicks.

Would it be possible to just have one set of tabs rather than sub-tabs? Perhaps with names:

- Full update
- Upload archive
- Export archive
- Single update
- Single export

This would keep the 3 pages related to "full" operations together, and hopefully make it clearer by glancing at the tab names, that the thing you export (an archive) can then be uploaded, and then you can update the config from that (or you might have put the config in staging in another way and update from that).

Personally I don't like the term synchronize at all by the way... I really don't know what it means, and given that there's not any UI text or help that uses the term "synchronize", I think it's not a good match for what the page does... So I thought of "Update", and then changed the Single page to have the same name.

(bikeshed away!)

jhodgdon’s picture

Actually I like #115 too... so maybe the tab names could be:

* Complete update
* Upload archive
* Export archive
* Update item
* Export item

tim.plunkett’s picture

Is "Update item" the new name for "Single import"? Because I use it to add config entities to the system, never tried updating an existing one...

jhodgdon’s picture

Good point. Probably "Import item" would be better. Not sure if it would do an update actually.

Eli-T’s picture

I've separated the confirmation dialog to #2572503: Add confirm dialog before full configuration import so that is not held back whilst the naming/arrangement of tabs is still being discussed.

Eli-T’s picture

Title: Reorder tabs in configuration UI and add confirm dialog before full import » Reorder tabs in configuration UI
Assigned: webchick » Eli-T
Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.03 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,901 pass(es). View

Patch to remove the confirm form from this issue now that's been moved to #2572503: Add confirm dialog before full configuration import.

This in no way is meant to disregard the suggestions by @jhodgdon, @yoroy and @tsvenson, just to cleanly split the two changes as they currently stand before doing further changes.

Eli-T’s picture

Status: Needs review » Needs work
Bojhan’s picture

@jhodgdon I am wondering what is the minimal we can do given the time-frame. This UI can always be optimised, but I feel we've already had over #100 comments and with that arriving at a sensible direction that will improve the user experience. I am a little uncomfortable with the very drastic suggestions, of completely dropping the arrived at information architecture.

I think we should retain the information architecture split between uploading and exporting. Moving to a "tab" per action is a big departure, and I am not sure it will actually improve the overview we are trying to create. I would suggest to primarily focus on making the labels more usable.

ifrik’s picture

Patch #122 looks good to me as it's very clearly seperates the different actions of importing and exporting, and only then let's the user choose between the same options (full or single) in both cases.

The hook_help text refers to the page titles by name so this either needs updating in this patch as well, or please post a follow-up issue as child of #1908570: [meta] Update or create hook_help() texts for D8 core modules.

Eli-T’s picture

@ifriks I've reviewed the current hook_help() implementation and don't think it needs any changes. Whilst the tabs have been renamed and restructured, the page names themselves haven't, and only the page names are referred to in hook_help().

yoroy’s picture

If we keep the tab organisation to what we arrived at and we update labels and texts in #2572537: Update the UI texts for the Configuration Manager module than this would be rtbc again. Is it practical to have a separate issue for the labels? I'm not so sure.

yoroy’s picture

Eli-T and I discussed how to best move forward with this and its related issues (yay face to face sprints :-).

- The tab arrangement we arrived at is a solid improvement, and we should commit this. But:
- Add one more label change here because that's what we've already been doing here: rename the 'Full' and 'Single' tabs to 'Full archive' and 'Single item'.
- Further refine help texts, field labels etc. in #2572537: Update the UI texts for the Configuration Manager module

Eli-T’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,901 pass(es). View

Patch to implement based on #128

yoroy’s picture

Assigned: Eli-T » alexpott
Status: Needs review » Reviewed & tested by the community

Tests pass, we're back to the earlier rtbc state with one additional change to the tab names as per #128.

#2572537: Update the UI texts for the Configuration Manager module and #2572503: Add confirm dialog before full configuration import are the followups, marking rtbc

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Given the amount of consensus building that has go on on this issue I think we're good to go. This makes the UI better and easier to understand and we have followups in place to continue the improvements. Committed 2c83881 and pushed to 8.0.x. Thanks!

  • alexpott committed 2c83881 on 8.0.x
    Issue #2247291 by Eli-T, yoroy, jhodgdon, alexpott, Bojhan, tim.plunkett...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.