Skip to content
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

User Dashboard - Center Report Card: Support #943

Merged
merged 18 commits into from
Jun 13, 2019

Conversation

ryanrath
Copy link
Contributor

Description

The changes made in this PR are in support of the novice_appkernels branch of the appkernels module. Specifically:

  • classes/Rest/Controllers/SummaryControllerProvider.php:
    • While testing I ran into problems w/ the $queryConfig->featured property not being present. Which caused exceptions to be thrown. So I just added a quick guard to make sure that the property was present prior to inspecting its value.
  • classes/XDUser.php:
    • The getResources function is utilized in the main novice_appkernels PR in the appkernels module, but as it made sense to include it in XDUser.php the change was made here.
  • html/gui/js/Viewer.js:
    • Eslint flagged this file for some reason so I just went through and reformatted it according to our current rules.
  • html/index.php:
    • These files are required by components included in the appkernels module which were moved from the internal dashboard to the main interface.

Motivation and Context

To get OpenXDMoD ready to support the CenterReportCardPortlet as well as host the AppKernelPerformanceMap from the internal dashboard.

Tests performed

Manual Testing was performed in conjunction w/ the App Kernels novice_appkernels branch installed. These changes are live on my dev port [9006].

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project as found in the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

jpwhite4 and others added 6 commits May 23, 2019 09:51
These files belong in the `appkernels` module and have been moved there.
Just cleaning things up to appease the linter.
This should not have been committed.
This change is included in the appkernels module branch `novice_appkernels`.
The inclusion of these files should be via AppKernels assets.d file.
@ryanrath ryanrath requested review from jpwhite4 and smgallo May 23, 2019 14:52
plessbd
plessbd previously approved these changes May 24, 2019
},

listeners: {
// eslint-disable-next-line no-shadow
Copy link
Contributor

Choose a reason for hiding this comment

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

part of me wants to just fix this instead of disabling the linter...

token = tabPanel.id + CCR.xdmod.ui.tokenDelimiter + tab.id;
Ext.History.add(token);
// eslint-disable-next-line no-mixed-operators
} else if (hasActiveTab && hasTab && tab.id !== CCR.xdmod.ui.activeTab.id ||
Copy link
Contributor

Choose a reason for hiding this comment

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

part of me wants to just fix this instead of disabling the linter...


if (token) {
var root = CCR.exists(parts.root) ? parts.root : 'main_tab_panel';
// eslint-disable-next-line no-shadow
Copy link
Contributor

Choose a reason for hiding this comment

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

part of me wants to just fix this instead of disabling the linter...

CCR.xdmod.ui.Viewer.viewerInstance = this;
}, // initComponent
getParameterByName: function (name, source) {
// eslint-disable-next-line no-param-reassign,no-useless-escape
Copy link
Contributor

Choose a reason for hiding this comment

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

part of me wants to just fix this instead of disabling the linter...

html/internal_dashboard/index.php Show resolved Hide resolved
Updates per @jpwhite4 code review comments

Co-Authored-By: Joe White <[email protected]>
@@ -2606,4 +2606,48 @@ public function isSticky()
{
return $this->sticky;
}

/**
* Retrieves the resources that this user has access to.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this function belongs in the XDUser class. It also circumvents the ACL tables - which surely is the authoritative source of user access information. Why not build on the existing getDimensionValues functionality in the Warehouse Controller? This already has the access control code and use the GroupBy classes to acces the data in the datawarehouse.

Copy link
Contributor Author

@ryanrath ryanrath May 28, 2019

Choose a reason for hiding this comment

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

*nod* It does circumvent the ACL tables. I'll look at the getDimensionValues function for a re-write. As an explanation, the results of this function are used in constructing a \AppKernel\PerformanceMap` object which utilizes the ACL tables to restrict the final output by the resources the requesting user has access to. But those changes are in the AppKernels module PR and I didn't reference it specifically. Thanks again for the catch, I'll have an update incoming shortly.

Ok, so quick update after actually reading my code ( I blame lack of caffeine ) ... This function doesn't circumvent the ACL tables in so much as it doesn't actually need them ( per se ). It uses this Users's organization_id to retrieve the resource_fact.id's associated with their organization. It's implied that if they are associated with an organization then they are also associated with their organization's resources.

So, since the function uses / requires User's Organization, I added it directly to XDUser instead of adding it as a helper function that accepts a $userOrganization parameter someplace else.

@jpwhite4 jpwhite4 added the Category:User Dashboard Screen shown after user login label May 29, 2019
ryanrath and others added 7 commits May 30, 2019 13:18
This is so that we can control when `$user->getAllRoles()` includes the public
acl. The default value for all arguments has been set to `true` as that was the
behavior pre-change. This will be used in the `AppKernels` module to help filter
appropriately for the Center Report Card Portlet.
We're no longer using this function so it is being removed.
Was missing a parameter from the `checkDataAccess` function call.
After much research and discussion w/ @jpwhite4, this function is being re-added w/ some
additional documentation && an authorization check to ensure that it is only
called for Center [Director|Staff] users. In addition, the REST endpoint that
uses this function ( in the `appkernel` module )is also being updated to require
users who request it to be Center [Director|Staff] as well.
Just needed to update the sql so that it returns data in the same format as
`MetricExplorer::getDimensionValues` as that is what `AppKernels/PerformanceMap`
is currently setup to process.
@ryanrath ryanrath merged commit bc90988 into ubccr:xdmod8.5 Jun 13, 2019
@plessbd plessbd changed the title Novice User Interface - Center Report Card: Support User Dashboard - Center Report Card: Support Oct 16, 2019
@plessbd plessbd added this to the 8.5.0 milestone Oct 16, 2019
@plessbd plessbd added the new feature New functionality label Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:User Dashboard Screen shown after user login new feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants