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

Version 2 of User/Organization Account Module #431

Merged
merged 9 commits into from
Jul 15, 2024

Conversation

muhammad-ali-e
Copy link
Contributor

@muhammad-ali-e muhammad-ali-e commented Jul 1, 2024

What

  • Created a new application with the same V1 methods, with some changes in the models.
  • Implementation of version 2 of the user/organization account module.
  • Refactoring to remove django_tenants and replace it with a new context management approach.
  • Updated the organization model and related methods for better structure and efficiency.

Why

  • Mandatory changes as part of the multi-tenancy design update.
  • To streamline the handling of organization contexts without relying on django_tenants.

How

  • Replaced tenant_context(organization) with new UserContext methods.
  • Updated the StateStore to store and manage the organization ID in the current context.
  • Refactored the organization model by removing TenantMixin and redundant methods.
  • Leveraged OrganizationMixin for better model structuring.

Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)

  • The new methods are self-contained ensuring no impact on current functionality.
  • Also under feature flag multi_tenancy_v2.

Database Migrations

Env Config

Relevant Docs

Related Issues or PRs

  1. unstract-multi-tenantV2 feature-flag changes #422
  2. Feat/multi tenancy v2 context and organization #430

Dependencies Versions

Notes on Testing

Screenshots

Checklist

I have read and understood the Contribution Guidelines.

@muhammad-ali-e muhammad-ali-e requested review from a team, jagadeeswaran-zipstack, ritwik-g, athul-rs, johnyrahul and hari-kuriakose and removed request for a team July 1, 2024 09:33
@muhammad-ali-e
Copy link
Contributor Author

I see Some sonar issues,

  • Duplication Codes:
    • The duplicated code is intentional, as we replicated the account application.
  • Security hotspot.
    • The current PR does not introduce any new changes in this area; it only involves copying existing code to the new application.
    • We can mark these security hotspots as acceptable since our application is running behind a proxy.
    • Any necessary changes to this code can be addressed in future updates, as my PR does not modify these functionalities.

@muhammad-ali-e muhammad-ali-e marked this pull request as ready for review July 1, 2024 09:45
Copy link
Contributor

@johnyrahul johnyrahul left a comment

Choose a reason for hiding this comment

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

LGTM overall. As this is v2 of same account app with changes mentioned in description.
Some of it dependent models will be raised as separate PR as author mentioned

@athul-rs
Copy link
Contributor

athul-rs commented Jul 1, 2024

@muhammad-ali-e One doubt, will we be removing the previous version after the test-runs ?
Until then are keeping the same duplicates ?

@muhammad-ali-e
Copy link
Contributor Author

@muhammad-ali-e One doubt, will we be removing the previous version after the test-runs ?
Until then are keeping the same duplicates ?

@athul-rs We will only remove the previous version once we have fully transitioned to the new feature and all QA checks are completed. Until then, we will keep all V1 applications, including their tables and schema, to ensure continuity and avoid any disruption.

Copy link
Contributor

@athul-rs athul-rs left a comment

Choose a reason for hiding this comment

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

LGTM for the overall code.
Only doubt remains on the duplicate code we are adding to OSS, please proceed if this is alright. cc: @hari-kuriakose

@hari-kuriakose
Copy link
Contributor

hari-kuriakose commented Jul 3, 2024

Bypassing Sonar duplication issue.

This is because the Django apps are duplicated purposefully, which is the less messier path. Similarly it's best the models too are separate in order to prevent any conflict with prior logic.

Also once we complete the migration, we will remove all the older Django apps.

@hari-kuriakose
Copy link
Contributor

We can take care of duplication when we delete v1 eventually. Bypassing.

Copy link
Contributor

filepath function $$\textcolor{#23d18b}{\tt{passed}}$$ SUBTOTAL
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_logs}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup\_skip}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_client\_init}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image\_exists}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config\_without\_mount}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_run\_container}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{TOTAL}}$$ $$\textcolor{#23d18b}{\tt{9}}$$ $$\textcolor{#23d18b}{\tt{9}}$$

Copy link

sonarcloud bot commented Jul 15, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
61.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@hari-kuriakose hari-kuriakose merged commit 8343f7a into main Jul 15, 2024
4 of 5 checks passed
@hari-kuriakose hari-kuriakose deleted the feat/MultiTenancyV2-account branch July 15, 2024 11:35
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.

5 participants