-
Notifications
You must be signed in to change notification settings - Fork 250
New credentials dialog #992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
ATH passing 🎉 Just need to review TODOs |
|
@jtnord would appreciate a review if possible, thanks. |
This comment was marked as resolved.
This comment was marked as resolved.
| --> | ||
| <?jelly escape-by-default='true'?> | ||
| <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:l="/lib/layout" xmlns:f="/lib/form"> | ||
| <st:setHeader name="X-Wizard-Title" value="${%Add Credentials}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
X-Wizard-Title?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sets the title of the wizard page - e.g. 'Add Credentials' -> 'Add GitHub App'
...esources/com/cloudbees/plugins/credentials/CredentialsStoreAction/DomainWrapper/dialog.jelly
Show resolved
Hide resolved
...sources/com/cloudbees/plugins/credentials/CredentialsStoreAction/DomainWrapper/dialog2.jelly
Outdated
Show resolved
Hide resolved
| checkMethod="${attrs.checkMethod}"/> | ||
| <!-- TODO add support for checking permissions against stores in request path --> | ||
| <j:set var="storeItems" value="${selectHelper.getStoreItems(context, includeUser)}"/> | ||
| <!-- TODO look into includeUser --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this valid? did things work (excepting permissions issues and failing hard) before and not now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see any difference. I don't think this works on master either.
I:
- Added a credential to a user store
- Configured a credentials parameter in a freestyle job.
- Clicked build with parameters
- Credential wasn't visible
- Ticked include user
- Credential still not visible
Same behaviour on both versions.
Unless you know any better I would just leave this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, it was just the TODO changed and I thought I had seen a commit around users.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Happens if you click inside the dialog but not on an actual element. Its because we're reusing a Copy the dialog code fully into here? Or a fix to core to allow an option to disable this? |
Resolved by updating the baseline to 2.528.3 |
Resolved by updating the baseline
Possibly, not a blocker though. |
@janfaracik possible to get an insight from you here? I've had a look at the DOM and it looks ok =/ |
src/main/resources/com/cloudbees/plugins/credentials/common/card.css
Outdated
Show resolved
Hide resolved
I've got this fixed in core - jenkinsci/jenkins#26033 |
|
@jtnord mind taking another look please? All should be fixed now, except for the closing of the dialog if you click in certain places. Short of forking the whole dialog code into credentials plugin we couldn't see a fix there and don't expect it to be a blocker to releasing this. |
credentials.mov
Part of #986
Fixes #939
ATH passed in jenkinsci/acceptance-test-harness#2565
TODO:
c:selectAdd - i.e. adding a credential from a credential chooser dropdownc:selectneeds migrating to wizard, i.e. overflow button needs to give domains in overflow...Testing done
nested folder example:

Tested with:
Submitter checklist