Improve UI for masking of variables in CI settings

Problem to Solve

Expanding on and inspired by gitlab-ce#13784, improve the user interface for creating and editing variables in CI settings by displaying the variables in a read-only table and moving editing actions into a modal.

Design solution

• Restructure UI section with a dedicated table (similar to new approvals UI)
• Adding variables will be done through a dedicated modal exposing the configurable options that way
• Validation can be done on submit (doesn't require to be done in real time, though nice if possible)
  • Masked variables are not accepted unless they fulfill regexp requirements
• Masking variable flag will be enabled by default
• Copy changes

Specs

What	Mockup
Project variable settings
Variable creation modal
Variable creation modal error

changed milestone to %11.10

added Category:Continuous Integration featureenhancement frontend + 1 deleted label

changed milestone to %11.11

added Deliverable label

added typefeature label

added 1 deleted label

assigned to @dimitrieh

mentioned in issue gitlab-org/frontend/release-kickoffs#2 (closed)

@dimitrieh I found out now that changes are being planned for the Variables section on Verify as well. We are working on the same section for Release as you can see here https://gitlab.com/gitlab-org/gitlab-ce/issues/46806. And for 12.0 we will need to improve the structure of the variables section to provide contextual variables - variable types request different fields https://gitlab.com/gitlab-org/gitlab-ce/issues/46806#note_158018456 - https://gitlab.com/gitlab-org/gitlab-ce/issues/57780 (please disregard the mockups in this issue).

I believe we missed the opportunity to sync and discuss an unified solution for the interface. Please keep in mind the requirements of the issues above for the UI you are proposing :)

Let me know if you have any questions!

cc @jlenny @brendan

Good point @rverissimo. Seems like we should hold off on these UI improvements during %11.11. If we work on them simultaneously, we would need to keep the gitlab-ce~3412464 efforts closely in sync.

@filipa @mfluharty what do you think?

This may cause some conflicts. Good catch @jhampton.

I'm transforming this list into a responsive table since we are adding headers in https://gitlab.com/gitlab-org/gitlab-ce/issues/46806

I'll do that in a separate MR to unblock things.

I'm wondering if it would make more sense to improve the UI in %12.0 to avoid conflicts

@rverissimo this seems to be the case indeed! Though this issue is something that was scoped off from a previous issue https://gitlab.com/gitlab-org/gitlab-ce/issues/13784 and tries to improve the IA in such a way that it becomes more flexible and extendible.

Imo it makes sense to delay this issue by one release in that case, as we'll be adding new functionality for 11.11 with gitlab-ce#46806. The IA problem becomes a little more obvious with each added thing though.

@brendan @jlenny can we agree to move this issue to 12.0 to fix the IA, but refrain from scheduling another issue that touches the same UI in that release?

cc: @filipa @jhampton

Thanks for catching this all. This may be something that we hit more often if we're not careful - maybe being clear about the "screens" where we overlap ~Verify and ~Release would be helpful (though the answer may be it always could be a problem if we're not careful).

Okay to move this to %12.0

Thanks @brendan!

added UX debt label

added devopsrelease [DEPRECATED] devopsverify labels

changed milestone to %12.0

Hi Team. I'm here as I've ran into confusion with the current "Masked" functionality, as there is no error message provided to the User letting them know why the variables can't be masked. I see from the above it needs to be RegExp compatible. This feature shouldn't have been published without proper UI/UX.

We're unlabeling ~"devops:release" because only one stage label should be used, and we keep the latest one which is: ~"devops:verify"

removed devopsrelease [DEPRECATED] label

mentioned in issue gitlab-org/frontend/release-kickoffs#3 (closed)

added UI polish label

This issue has passed the feature freeze date and considered a gitlab-ce3804821. Adding gitlab-ce10824706.

added missed-deliverable missed:12.0 labels

Removing the missed labels

removed missed-deliverable missed:12.0 labels

This issue has passed the feature freeze date and considered a gitlab-ce3804821. Adding gitlab-ce10824706.

added missed-deliverable missed:12.0 labels

Now that %12.0 has shipped, this issue appears to have been missed. Since it is in progress I'm moving it to the %12.1 release. cc @erushton @mfluharty @jhampton

Thanks @jlenny.

@mfluharty if you have concerns about completing this in %12.1, please reach out to @shampton. He should have some capacity to help out. Thanks!

changed milestone to %12.1

CC @jhampton

changed milestone to %12.2

Postponing this due to conflict with https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/30509, which is modifying the same UI

cc: @jhampton

@mfluharty this UI can be considered an upgrade over the improvements made by https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/30509.

In that merge request, I actually asked @tnir to consider this issue or https://gitlab.com/gitlab-org/gitlab-ce/issues/61008 as a follow-up iteration ;).

Because of that, I see no reason to block this issue as this should ultimately replace the current UI , please consider not postponing this.

@mfluharty We skipped this item in our meeting today. Is this on your radar? This issue might impact future work if (not) implemented.

@mfluharty thanks for the update on slack, can you please provide your feedback in the issue when you get around to it?

changed milestone to %12.3

added grouppipeline execution label

fyi @mnichols1 - since we also work on top of variables

changed milestone to %12.3

This item was unfortunately not completed in time for the %12.3 cutoff; moving to %12.4 where it will be finished.

changed milestone to %12.4

added missed:12.3 label

mentioned in issue #28321 (closed)

mentioned in issue #17069 (closed)

mentioned in issue #24172 (closed)

added workflowready for development label

@jlenny @dimitrieh we are having difficulty prioritizing this enhancement in %12.5. Tentatively moving this into %12.6 for consideration from Product. Please adjust the milestone if needed.

This issue fixes important usability problems for the variables project settings section, therefore it is important to prioritize. Additionally due to this not getting into 12.4 #24172 (closed) will ship with inconsistent UI until this gets implemented.

changed milestone to %12.6

added missed:12.4 label

unassigned @mfluharty

removed Deliverable label

@jlenny @thaoyeager will this be reconsidered for %12.7. This fixes important usability problems to a UI area where we have invested a lot of new feature changes/additions over the last half-year.

@dimitrieh what do you mean, you want it moved out of %12.6? I'm not following exactly.

@jlenny I thought this was not a real deliverable for 12.6 as it misses the Deliverable label. However, I do see the workflowready for development label, which does indicate engineering commitment, correct? If so, this is fine

@dimitrieh at the time planning wasn't done. It does look like this didn't make it in the end, though.

unassigned @dimitrieh

added [deprecated] Accepting merge requests label

removed 1 deleted label

removed milestone

Per discussion with @dcipoletti, this issue has not been committed to by CI Frontend team (hence no Deliverable label) and will need to wait until some other frontend work on a related issue is completed.

@thaoyeager we should either move this to the %Backlog milestone or a different release, having no milestone at all usually means it's untriaged.

@thaoyeager following up on this one - could you please take a look?

@jyavorska I am updating this issue with %Backlog and will also ask Eng to estimate for %12.8.

Updated the description mockup urls

mentioned in issue #26463 (closed)

marked this issue as related to #26463 (closed)

changed milestone to %Backlog

added workflowplanning breakdown label and removed workflowready for development label

mentioned in issue verify-stage#9 (closed)

added UX label

changed the description

@dimitrieh some feedback on the UX here:

1. I suggest moving the action buttons to the top of the list. Currently, they are positioned on the bottom of the list, and will be pushed once more variables are added, making it difficult for users to manipulate the list. See image below:

2. can you include a responsive view to this list? We currently have a problem with the UI that does not behave so nicely on a small viewport. It would be nice to include here some of the UI polish work as well.

3. The dropdown needs to be improved to display a list of existing environments to the user. This bug was reported in an issue for ~"devops::release" and I suggest making the following addition to the UI/UX: #26463 (comment 268217004). We can work on it separately but it would be nice to know that this change will be included in your acceptance criteria.

WDYT?

fyi @jmeshell

@dimitrieh @rverissimo Agreed that it would be great to see a UX spec for a responsive view!

A few other thoughts/questions:

1. It would be great to include some large variable keys/values in the UX specs, since this is a pain point with the current design. We should make sure to define how the fields should expand/rearrange/truncate in response to large values.
2. What's the advantage of moving the input into a modal dialog? I feel like this might make entering lots of variables more tedious than it is today.
   1. If we do move forward with a modal dialog, we should give special care to the keyboard focus to allow multiple variables to be added and saved entirely from the keyboard.

@nfriend

What's the advantage of moving the input into a modal dialog? I feel like this might make entering lots of variables more tedious than it is today.

We didn't validate this proposal with users yet so I'd say the trade-off is really around decluttering the UI and moving the managing of the variables to a different view. But I'd love to test this proposal out and see how people actually want to interact with it. @jmeshell we spoke about it this week, perhaps something we can take over to ~"devops::release" ?

Sounds good @rverissimo, how can I help move this along? Do we need to confirm with the PM of ~"group::continuous integration"?

@jmeshell Yup, I'd say confirming the ownership here would be a great first step. Perhaps we can partner up and do the validation together. I'd love to experiment with some cross-functional ux collab @dimitrieh

@rverissimo just came across this thread. Are we implying that ~"devops::release" is picking up this issue?

cc: @thaoyeager

@pburdette from a UX point of view, yes. We can partner up and do the workflowdesign together with devopsverify. Not sure how the development phase/ownership would look like, though. But I don't see this as a blocker for the design phase.

@rayana @jmeshell @dcipoletti

I went ahead and brought in the environments api endpoint and the environment scope dropdown is populated in a list format. We can follow up on an iteration to improve UX on that such as listing alphabetically etc. But the bulk of that work is already in place now.

!25263 (diffs)

@dimitrieh @rverissimo @jmeshell Does it make sense to mark one of these as a duplicate of the other?

• #27480 (closed) (this issue)
• #28296 (closed)

It looks like they link to the same design proposal.

@nfriend - I agree - I will let @dimitrieh pull the trigger on that

marked #28296 (closed) as a duplicate of this issue

added 1 deleted label

added direction label

removed direction label

mentioned in issue #26777 (closed)

Per discussion with @dcipoletti, FE engineering can commit to this issue for %12.8 .

cc: @pburdette

changed milestone to %12.8

added Deliverable workflowready for development labels and removed workflowplanning breakdown label

assigned to @pburdette

@dimitrieh are we getting rid of variable type here? I'm not seeing it in the new designs?

@pburdette no we are not . Very important also for vault #20306

My suggestion is to add in an extra column at the beginning of the table and an extra dropdown at the top inside of the creation modal!

Would you be able to work with that or do you need some wireframes to clarify?

@dimitrieh no that's good enough, just wanted to make sure.

unassigned @pburdette

Un-assigning myself until it's determined if devopsverify or ~"devops::release" will be picking this issue up for 12.8.

cc: @jmeshell @thaoyeager

@pburdette this won’t be in %12.8 for ~"group::release management" per @thaoyeager’s comment in #27480 (comment 271411838) this looks like it’s devopsverify and ~"devops::release" will take a related bug

@jmeshell Thanks for the update. Just curious, what is the related bug? Might be good for me and @pburdette to be aware of it to avoid change collisions.

@thaoyeager this should be coordinated in this comment #27480 (comment 268221260)

@jmeshell @thaoyeager thanks for the clarification everyone

assigned to @pburdette

removed [deprecated] Accepting merge requests label

mentioned in issue #198661 (closed)

@rayana based on #26463 (closed) it seems your team will be picking the issue with the environment_scope dropdown. So should I leave this dropdown as a static dropdown with one option of All environments ?

FYI, I'm looking into pre-filling the dropdown with the correct environment data right now.

cc: @dimitrieh

@rayana @dimitrieh I tracked down the endpoint to get predefined environments for the environment_scope dropdown. It's being pre-filled with that data now.

@dimitrieh a few questions...

FYI, I'm rebuilding this feature in Vue. And I'm using the gl-table component.

1. When a user clicks the edit icon for a variable, is it okay if the edit ability opens up in a dropdown with the data pre-filled in the modal?
2. Why do we want to have masked enabled by default? Since it's based on the variables value, if we tracked it real time they would immediately see error messages since the value doesn't meet our criteria for masked since there will be no value present.

For question 2, I disable the masked checkbox until the regex requirements are met for the value field. After the requirements are met, it enables real time.

Thoughts?

@pburdette

When a user clicks the edit icon for a variable, is it okay if the edit ability opens up in a dropdown with the data pre-filled in the modal?

I am not exactly sure I get what you are saying .

"User clicks Edit icon" => "Opens a dropdown" => "also gets a modal"?

Data prefilled sounds right!

1. Why do we want to have masked enabled by default? Since it's based on the variables value, if we tracked it real time they would immediately see error messages since the value doesn't meet our criteria for masked since there will be no value present.

For question 2, I disable the masked checkbox until the regex requirements are met for the value field. After the requirements are met, it enables real time.

Your solution sounds good! Basically, when the user has not yet started editing the error box will not show up yet, correct?

@dimitrieh

Sorry I meant to say that when the edit icon is clicked on a variable it will open up a modal with the data to be edited. I was just making sure we didn't want to have the edit icon change the static table of information into an editable list of DOM nodes. This would complicate things a good bit.

I'll provide you a screen recording to visually explain what I'm talking about for the masked feature.

@pburdette yes no direct editing in the table . What you suggest aligns perfectly with the proposal of this issue

@dimitrieh what state do we want to show when there are no variables present in the table?

@pburdette Ad discussed let's show an empty state within the table similar to

assigned to @dimitrieh

Given current progress in %12.8 and that this is still in planning breakdown I don't expect this to be completed for this milestone. For now I am moving to %12.9 where it will need to be evaluated again amongst the other priorities there (and progress on %12.8) as we get closer to commitment for that release.

@jyavorska apologies, the workflow label should have been in development. I hope to have this MR wrapped up by end of week and in review. !24088 (closed)

Ok sure, moving back.

Update: This should be all in by end of week, as long as maintainers can review in a timely manner. Had to break up my larger WIP MR into four smaller MRs to keep reviews light.

MR 1: !25260 (merged) Introduce Feature Flag

MR 2: !25263 (merged) Introduce Vuex store / specs

MR 3: !25468 (merged) Introduce components / specs

MR 4: !25731 (merged)

Follow up Issue to remove old UI and relative specs: #207827 (closed)

cc: @dcipoletti @jyavorska

@sarahghp pinging you on update since this code is needed for AWS issue you have.

All code is in master now! So any fresh branch or review app will have these changes enabled by default. Old code is still present at the moment until we confirm this feature is 100% working the way we want it too. That way we can easily roll back to the Haml version (old UI) at any point right now.

Also here is a video going over UX https://youtu.be/5N2WuXEGHSM

Once UX approves current changes I'd say we are good to close this. We can make some UX adjustments before the next release, If UX has any.

cc: @dcipoletti @rayana @dimitrieh

Thanks for the update here @pburdette Very convenient in the video format!

In terms of functionality, this seems great! I do have a concern for the masking regex requirement implementation as shown in the video. It is very subtle and doesn't communicate to the user what is needed apart from the checkbox description.

Perhaps we can add make that part of the description bold when disabled. Wdyt?

Also, what happens if you enable masking, but then alter the variable value to not be inline with the regex requirements? (I would expect an error message as described in the description of this issue)

changed milestone to %12.9

added workflowin dev label and removed workflowready for development label

changed milestone to %12.8

added VerifyP1 label

removed Deliverable label

added workflowin review label and removed workflowin dev label

changed milestone to %12.9

added missed:12.8 label

I think the wording for the Protect variable option is wrong or misleading. It's not "allowing" it to run on protected branches and tags. It's restricting it to only be available to pipelines that are running on protected branches or tags.

@marcel.amirault can you provide some help with any copy adjustments for this feature?

@pburdette Just found this ping in my backlog, sorry for missing it!

Glad we got it figured out in the MR though, this is a great UI improvement!

unassigned @dimitrieh

This looks to now be live on Canary, and it's got some issues.

This is what I see in release-tools:

There's no indication that there's horizontal overflow.

@rspeicher how I can view what you're seeing?

Showing strange for me too. All variables are listed as type File when in fact they are not. It's still working. [image: Screen Shot 2020-02-26 at 11.29.34 AM.png]

@pburdette These are the variables in https://gitlab.com/gitlab-org/release-tools/-/settings/ci_cd, however you may not have access to view that project's settings.

I think the problem may be a long variable name. The longest we seem to have is OMNIBUS_SINGLE_BRANCH_VERSION (29 characters)

The problem is exacerbated when 'Reveal values' is clicked, as then I pretty much only see the key and the value before having to scroll, since almost all of these values are long strings without spaces.

Here's a recreation.

One variable with a reasonably short name:

And then when we add a second one with a longer name, the problem can be seen:

And even worse when we show values:

If this is behind a feature flag I'd say disable it; if it's not, I'd say revert before this is on production and causing confusion.

Also, for some reason in the modal we're defining select fields with a fixed height: 34px rule, which causes a cutoff if the browser is using anything other than the default Medium font size (I use Large on a 4K display):

Feature flag has been disabled: https://gitlab.com/gitlab-com/gl-infra/feature-flag-log/issues/503

We can return to the default enabled behavior via /chatops run feature delete new_variables_ui.

Thanks a ton @rspeicher! I'll look over these edge cases and apply some fixes.

@bdowney I'm not seeing any problems on my end when fetching variables on load. If they are sent up as env_var or file they should come down properly. Are you positive when these variables were created they were the env_var type? Thanks

@dimitrieh how should we handle long variable names / variable values here? Instead of the horizontal overflow?

We have a lot of use cases and are possibly have to cram really long values into this table.

Thread

We could make the table a fixed width and the columns overflow: hidden with text-overflow: ellipsis

So a really long key would be REALLY_LONG_KEY...

@dimitrieh WDYT? But the only way to see what the key is right now would be to edit the variable, which isn't the best. Or we possibly could do a tooltip to show the full key/value when hovering a value that is hidden due to its width.

@pburdette the design intended for the columns for some to be a fixed width and other to fill up the remaining space in certain percentages. This way horizontal scrolling of the entire table does not happen.

On mobile, the table converts a mobile table.

For long values, if it exceeds the space overflow: hidden with text-overflow: ellipsis is the solution I had in mind. If possible, can we make the complete value show up in a pop-over together with a copy to clipboard button like so (This popover will show regardless of the value being ellipsed):

Let me know what you think!

PS: also see my comment at #27480 (comment 294367692)

@pburdette 100% sure they were setup as Variable and not File. But now they are looking correct.

@dimitrieh yes we can do that. I assume we want to do this for both key and values? But only values once the Reveal values button is clicked?

I just saw the behavior Brad saw above. I think if you go to group-level CI variables, they all show as File.

Thanks @bdowney and @rspeicher

I see that as well, investigating now

So they were all showing "File" because that's default if nothing is returned from the endpoint. And it looks like the controller for groups isn't sending back the variable type in the response.

Network response for groups

Already mentioned this to Payton but adding here for the record, the variables are not in any discernible order:

Thanks @rspeicher just applied a fix for that locally to make them in ascending order

@pburdette for documentation purposes I am listing some of the problems with the current merged result after doing a quick review:

• Amount of used space for the table is quite small, it seems to be less wide than before
• IA problems:
  • Ordering of variables (on what is this currently based?)
  • Variable values seem to be leveled with less important data such as var type etc
  • true/false is harder to read on a glance (might need to swap for colored icons)
  • Environment scope value takes to much space with default value all environments
  • We need to test all columns with big information loads
  • Some columns do not have enough space for their content (like the buttons)
• Modal size and information order might need to be relooked at

I propose that we revert the current implementation and give this a relook both from a conceptual and engineering PoV. Also, see the slack discussion at https://gitlab.slack.com/archives/CPCJ8CCCX/p1582882626042100

cc: @dcipoletti @thaoyeager

@dimitrieh it's behind a feature flag that was disabled #27480 (comment 294885416)

Amount of used space for the table is quite small, it seems to be less wide than before

It's actually the same width, both tables are in a col-lg-12. But I thought it was quite small to, I like the width on the groups settings better But that would require an entire refactor of the entire project settings layout.

Variable values seem to be leveled with less important data such as var type etc

Do you propose we leave out the variable type from the display table? This will help a lot with space. I know it wasn't in the mockup for the table or modal, but as we discussed this was a mistake as it's needed for adding a variable but maybe not needed for users to view in a table.

Ordering of variables (on what is this currently based?)

My new MR opened !26098 (merged) orders these now by key in ascending order

true/false is harder to read on a glance (might need to swap for colored icons)

Let's do this, I had this in mind as well. What icons?

Environment scope value takes to much space with default value all environments

Agreed, not sure if we can explain the scope with an icon for All environments ?

I have an MR to fix / discuss all of this opened here !26098 (merged)

While it's good it's behind a feature flag, it's defaulted to enabled so if we can't address everything before we're close to a 12.9 release, it will need to be reverted or change the default feature flag value.

@rspeicher I'll open an MR to remove default true to be extra safe.

MR here @rspeicher !26020 (merged)

@dimitrieh

I removed the variable type. And set the columns protected & masked to a set width of 100px and the remaining columns to a equal width of 180px. This helps spacing a good bit. WDYT? Variables are ordered by key in ascending order.

cc: @dcipoletti

The border issue has been merged and will now be available in GitLab UI: gitlab-ui!1116 (merged)

@dimitrieh Thanks for your guidance thus far on this. It looks like until we have the above questions answered that frontend is blocked. Looking forward to your expertise!

@dimitrieh I assigned you to !26098 (merged) for UX review

I addressed some of your concerns in that MR where we can tweak and adjust. I also left a video in MR description going over the changes.

@pburdette thanks for creating the merge request! We actually have a lot to fix it seems and the problems need to be discussed individually in detail.

I propose we do that in the merge request !26098 (merged)

I created sub-threads for each problem including suggestions on how to fix it! I propose we continue there!

cc: @dcipoletti

assigned to @dimitrieh

mentioned in merge request !26683 (merged)

Closing based on !26098 (merged)

closed

@pburdette I've again disabled the feature flag on production due to a major bug:

charlie ablett:charlieshat: Today at 3:37 PM
Ruby colleague of mine found this bug, introduced in the last 12h: “If you open a CI variable to edit it, if you x out of the popup, it removes the variable completely”. Is this known?

4 replies

Robert Speicher  5 minutes ago
cc @pburdette

Robert Speicher  3 minutes ago
Wow, it deletes the variable even on the backend. The hell?

Robert Speicher  2 minutes ago
https://gitlab.slack.com/archives/C101F3796/p1584391225445100

GitLab Chatops
The feature flag value has been updated!
Posted in #production | Today at 3:40 PM | View message

Robert Speicher  1 minute ago
@charlie I've disabled the flag globally on production, please thank them for finding that.

Testing myself, when you close the modal it sends this PATCH request:

{"variables_attributes":[{"id":1836206,"key":"test","value":"test","variable_type":"env_var","protected":"false","masked":"false","environment_scope":"*","secret_value":"test","_destroy":true}]}

Notice the _destroy=true value.

Thank you to @idanoo who originally identified the bug!

Bug can be found here !26098 (diffs) which traces back to here https://gitlab.com/gitlab-org/gitlab-ui/-/blob/master/src/components/base/modal/modal.vue#L124 in gitlab-ui

It's due to the modals x button also triggers a secondary event.

mentioned in merge request !27364 (merged)

added depth label

mentioned in issue #5313 (closed)

Guys, what do you waiting for, push it!

mentioned in issue #24575 (closed)

mentioned in issue #267592