Refactor fields.inc to be configuration only (CFM-218) #342
Conversation
app/availableplugins/ApiConnector/templates/ApiSources/fields.inc
Outdated
Show resolved
Hide resolved
| @@ -74,6 +75,24 @@ public function initialize(array $config): void { | |||
| $this->setPrimaryLink(['external_identity_source_id']); | |||
| $this->setRequiresCO(true); | |||
|
|
|||
| // All the tabs share the same configuration in the ModelTable file | |||
| // XXX Reenable when this doesn't throw an error (same error as https://todos.internet2.edu/browse/CFM-475 ) | |||
There was a problem hiding this comment.
I attempted to add the subnavigation for API Sources config similar to File Sources, Sql Sources (etc). When this block is included it throws an error similar to what we see in https://todos.internet2.edu/browse/CFM-475
There was a problem hiding this comment.
What's the plan to address this? ie: CFM-475 is marked resolved, so either we should have a plan to address the underlying issue or we shouldn't commit a bunch of commented out broken code.
There was a problem hiding this comment.
I will pull this code out for now. We'll need to review all such places where subnavigation should be used - where configuration (for example) should be on a tab next to its entity.
arlen
force-pushed
the
feature-cfm218-fieldsIncConfigOnly
branch
2 times, most recently
from
984681b
to
46b3261
Compare
arlen
changed the title
arlen
force-pushed
the
feature-cfm218-fieldsIncConfigOnly
branch
from
29c718b
to
620cdf5
Compare
arlen
force-pushed
the
feature-cfm218-fieldsIncConfigOnly
branch
2 times, most recently
from
1856b00
to
c7b8668
Compare
arlen
force-pushed
the
feature-cfm218-fieldsIncConfigOnly
branch
from
c7b8668
to
3cae121
Compare
|
Moved this PR to draft mode while refactoring for recent updates to develop. Due to the refactoring, it has also become clear that CFM-465 (the ability to assign 'type' to a banner/alert message) is required. I will take this back out of draft mode when complete and pushed. |
arlen
force-pushed
the
feature-cfm218-fieldsIncConfigOnly
branch
from
0618b10
to
e015a94
Compare
|
This PR has been rebased against the lastest develop and opened back up for review. |
…standard way (CFM-218)
| @@ -74,6 +75,24 @@ public function initialize(array $config): void { | |||
| $this->setPrimaryLink(['external_identity_source_id']); | |||
| $this->setRequiresCO(true); | |||
|
|
|||
| // All the tabs share the same configuration in the ModelTable file | |||
| // XXX Reenable when this doesn't throw an error (same error as https://todos.internet2.edu/browse/CFM-475 ) | |||
There was a problem hiding this comment.
What's the plan to address this? ie: CFM-475 is marked resolved, so either we should have a plan to address the underlying issue or we shouldn't commit a bunch of commented out broken code.
| foreach([ | ||
| 'env_identifier_sourcekey', | ||
| 'env_address_street', | ||
| 'env_address_locality', | ||
| 'env_address_state', | ||
| 'env_address_postalcode', | ||
| 'env_address_country', | ||
| 'env_affiliation', | ||
| 'env_department', | ||
| 'env_identifier_eppn', | ||
| 'env_identifier_eptid', | ||
| 'env_identifier_epuid', | ||
| 'env_identifier_network', | ||
| 'env_identifier_oidcsub', | ||
| 'env_identifier_samlpairwiseid', | ||
| 'env_identifier_samlsubjectid', | ||
| 'env_mail', | ||
| 'env_name_honorific', | ||
| 'env_name_given', | ||
| 'env_name_middle', | ||
| 'env_name_family', | ||
| 'env_name_suffix', | ||
| 'env_organization', | ||
| 'env_telephone_number', | ||
| 'env_title' | ||
| ] as $field) { |
There was a problem hiding this comment.
You can avoid duplicating the set of field names by doing something like this instead:
foreach($defaultNames as $field => $envName) {
$fields[$field] = [
'default' => $envName
];
}
There was a problem hiding this comment.
Good point. I think this was a direct conversion of the original file - it's nice to see things getting even further simplified. I've updated the code in the latest commit.
| msgid "add.a" | ||
| msgstr "Add an SSH Key" | ||
|
|
There was a problem hiding this comment.
Where is this used, and why is it not using __d('operation', 'add.a', [__d('ssh_key_authenticator', 'controller.SshKeyAuthenticators')])?
There was a problem hiding this comment.
I've pulled this out. I suspect this may have been left behind after a rebase.
app/resources/locales/en_US/field.po
Outdated
| msgid "Groups.members_group_id" | ||
| msgstr "Members Group" | ||
|
|
There was a problem hiding this comment.
I've also removed this, and I also suspect a rebase miss.
The point of this string was to replace "Owners for Group" label that exists where an Owners group has the link back to the Members Group. I find the existing label a little confusing to read and felt "Members Group" was a better label. We can discuss - but it doesn't need to be part of this PR.
(Almost all labels name the field they represent in a simple way. "Owners for Group" refers back to the current group loaded in the form - it's unusual...and inconsistent.)
app/templates/Groups/fields.inc
Outdated
| $mveasEntityType = "group"; | ||
|
|
||
| // XXX: if CFM-218 (Make fields.inc configuration only) is accepted, move the contents of this file into fields.inc |
There was a problem hiding this comment.
The comment there was accidentally left in from the copy and paste out of fields-nav.inc. It's been removed.
No description provided.