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

Extract NeoDash auth module to an abstract interface to allow for different providers #767

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

alfredorubin96
Copy link
Collaborator

Detaching the query and authentication logic to a new class

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: Patch coverage is 35.26786% with 145 lines in your changes are missing coverage. Please review.

Project coverage is 37.36%. Comparing base (19b7538) to head (e7d7d98).
Report is 10 commits behind head on develop.

❗ Current head e7d7d98 differs from pull request most recent head 4895d8b. Consider uploading reports for the commit 4895d8b to get more accurate results

Files Patch % Lines
src/dashboard/DashboardThunks.ts 11.11% 64 Missing ⚠️
src/extensions/rbac/RBACUtils.ts 0.00% 24 Missing ⚠️
src/connection/ConnectionModule.ts 18.75% 13 Missing ⚠️
src/connection/neo4j/Neo4jConnectionModule.ts 31.25% 11 Missing ⚠️
src/connection/neo4j/utils.ts 79.41% 7 Missing ⚠️
...oard/sidebar/modal/DashboardSidebarAccessModal.tsx 22.22% 7 Missing ⚠️
src/connection/neo4j/runCypherQuery.ts 14.28% 1 Missing and 5 partials ⚠️
src/application/logging/LoggingThunk.ts 0.00% 5 Missing ⚠️
src/extensions/forms/chart/NeoForm.tsx 0.00% 2 Missing ⚠️
src/extensions/rbac/RBACManagementMenu.tsx 0.00% 2 Missing ⚠️
... and 3 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #767      +/-   ##
===========================================
- Coverage    37.49%   37.36%   -0.14%     
===========================================
  Files          216      220       +4     
  Lines         9140     9274     +134     
  Branches      2703     2730      +27     
===========================================
+ Hits          3427     3465      +38     
- Misses        5654     5745      +91     
- Partials        59       64       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

sonarcloud bot commented Jan 29, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@nielsdejong nielsdejong changed the title Feature/auth module Extract NeoDash auth module to an abstract interface to allow for different providers Mar 19, 2024
throw new Error(`Not Implemented: ${functionName}`);
};

export abstract class ConnectionModule {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add some docstrings here, since it's a pretty important interface we should document it well

throw new Error(`Not Implemented: ${functionName}`);
};

export abstract class NeodashRecordParser {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will break right? bulkParse for tables?

Maybe we can just leave out the record parser in this iteration of the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

from testing didn't looked like it was breaking, but we can just remove it to be 100% sure.

}

loadDashboard(_id: string): any {
return notImplementedError('loadDashboard');
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we including dashboards CRUD operations in the current scope or not yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally we should, but let's focus first on migrating the format of the records, i think that's more urgent. In the current release we should just create the base for the next releases.

@@ -303,70 +303,71 @@ export const loadDashboardFromNeo4jThunk = (driver, database, uuid, callback) =>

try {
const query = 'MATCH (n:_Neodash_Dashboard) WHERE n.uuid = $uuid RETURN n.content as dashboard';
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should manually check that all these operations (dashboard load via share link, dashboard load via sidebar, dashboard load in readonly mode) are all still functional. There's quite some breaking points that we should double check before pushing out the next release.


const setRecords = (records) => {
let toSet = records;
if (['table'].includes(type)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this since it's not finished?
Add a todo for later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah we can do it, but from next release we must start the migration to the new abstracted model

Copy link

sonarcloud bot commented Mar 28, 2024

Quality Gate Passed Quality Gate passed

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@alfredorubin96 alfredorubin96 self-assigned this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants