Skip to content

Addition of Person Overview/Canvas (CFM-205) #62

Closed
wants to merge 6 commits into from

Conversation

arlen
Copy link

@arlen arlen commented Nov 21, 2022

Commit also includes reworking of subnavigation for a Person view, VueJS starter components, updates for AJAX API access, and a minor fix to textarea rendering.

@arlen arlen requested a review from benno November 28, 2022 19:52
Comment on lines 600 to 604
'ApiUsers.username' => $this->authenticateApiUser,
'ApiUsers.username' => $this->authenticatedUser,
'ApiUsers.co_id' => $coId,
'ApiUsers.status' => SuspendableStatusEnum::Active
])
->contain()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you meant to commit this?

Copy link
Author

@arlen arlen Dec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did mean to commit this - this is the change found in PR-61 as well, and I wasn't sure which would be addressed first. The API user find for ajax requests doesn't work without it (at the moment). Were this merged, it obviates PR-61.

However, this also was added because I could only make API calls by creating an API user, and maybe there's a better approach. We'd need one preinstalled at setup otherwise so that the API ajax calls can work out of the box.

Comment on lines +104 to +105
// This contain() directive is the primary deviation from standard edit()/view().
// We need to drill down to deeper related models for display.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this be configured using QueryModificationTrait's setEditContains() and setViewContains()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Perhaps it can?

Comment on lines +313 to +336
// XXX This redirects to an Exception page because $id is not found.
// XXX A 404 with error would be better.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but is adding this comment necessary for this commit? (Same below.) We could open a new JIRA for improving error rendering.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - it's not necessary - I just didn't want to forget about it. I'll add a jira and remove these.

@@ -46,7 +46,7 @@ trait PrimaryLinkTrait {
private $unkeyedActions = ['add', 'index'];

// Actions where the primary link can be obtained by looking up the record ID
private $lookupActions = ['delete', 'edit', 'view'];
private $lookupActions = ['delete', 'edit', 'canvas', 'view'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we specifically remove the canvas action? ie: There's no need for PeopleController to have both an edit and a canvas action, and canvas just requires a bunch of special case handling, so why not just implement the canvas functionality under edit? (I'm pretty sure we decided this a few months ago.)

I realize this will complicate the view file, below, but we can handle that pretty generically by eg allowing the controller to not use the standard view.

Copy link
Author

@arlen arlen Dec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, we kind of need both because of the way the tabs work under the overview. Edit = "properties" tab. There's a lot to be done to move to a complete ajax model, and I'd like this stuff to work in the meantime. There are things to like about the current approach, and we should do some A/B comparisons before we fully commit. (I'm going to work up some miro boards as well.)

$linkTitle = __d('information','global.title.none');
switch($mveaType) {
case 'names':
/* XXX THIS IS TEMPORARY. This approach for generating the name label
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If $mvea is the actual name object (as opposed to an array of data), you can just call $mvea->full_name. Let's fix this so we don't end up with duplicate logic that we never get around to cleaning up.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

case 'addresses':
$linkTitle = trim($mvea['room'] . ' ' . $mvea['street']);
break;
case 'telephone_numbers':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, TelephoneNumber->formatted_number. It might make sense to standardize the magic function name that creates the formatted version so we don't have slightly different virtual field names for the different MVEAs.

]
); ?>
<?php if($mveaType == 'addresses'): ?>
<?php // XXX if we have other places to output an address just for display, make a function. ?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No "if" here, we should add a similar function to the Address Entity. To start, in can mimic the v4 functionality.

@arlen arlen marked this pull request as draft December 6, 2022 20:08
@arlen
Copy link
Author

arlen commented Dec 6, 2022

Converted this to draft: we will take the developed work here and carry it in full to the ajax approach which is partially implemented here.

@arlen arlen force-pushed the feature-cfm205-personOverview branch from 8c7fa95 to 7c6d66b Compare February 7, 2023 17:26
@arlen arlen force-pushed the feature-cfm205-personOverview branch from 7c6d66b to dead5ff Compare February 7, 2023 22:19
@arlen
Copy link
Author

arlen commented Feb 8, 2023

This PR will be closed while we work towards the full AJAX based person canvas in branch feature-cfm205-personOverview. See the original ticket at https://todos.internet2.edu/browse/CFM-205

@arlen arlen closed this Feb 8, 2023
Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants