added schema and crl apis for organisation #322

sauraww · 2024-12-20T08:35:27Z

Problem

Describe the problem you are trying to solve here

Solution

Provide a brief summary of your solution so that reviewers can understand your code

Environment variable changes

What ENVs need to be added or changed

Pre-deployment activity

Things needed to be done before deploying this change (if any)

Post-deployment activity

Things needed to be done after deploying this change (if any)

API changes

Endpoint	Method	Request body	Response Body
API	GET/POST, etc	request	response

Possible Issues in the future

Describe any possible issues that could occur because of this change

Datron · 2024-12-20T08:47:59Z

crates/superposition/organisation/Cargo.toml

+name = "organisation"
+version = "0.1.0"
+edition = "2021"
+
+
+[dependencies] 
+service_utils = { path = "../../service_utils" } 
+superposition_types = { path = "../../superposition_types", features = [
+    "result",
+    "diesel_derives",
+]}
+serde = { workspace = true }
+serde_json = { workspace = true }
+chrono = { workspace = true } 
+actix-web = { workspace = true } 
+anyhow = { workspace = true } 
+diesel = { workspace = true , features = ["numeric"]}
+idgenerator = "2.0.0"
+log = { workspace = true }
+superposition_macros = { path = "../../superposition_macros" }


We don't need a crate for org, it can just be a module

Datron · 2024-12-20T08:51:02Z

crates/superposition/organisation/Cargo.toml

+actix-web = { workspace = true } 
+anyhow = { workspace = true } 
+diesel = { workspace = true , features = ["numeric"]}
+idgenerator = "2.0.0"


idgenerator seems to use snowflake internally, can we just use the snowflake generator internally?
https://crates.io/crates/idgenerator

Datron · 2024-12-20T08:51:39Z

crates/superposition/organisation/src/api/handlers.rs

+    mut db_conn: DbConnection,
+) -> superposition::Result<HttpResponse> {
+    let org_id =
+        db_conn.transaction::<_, superposition::AppError, _>(|transaction_conn| {


Do we need a transaction for a single insert?

Datron · 2024-12-20T08:52:09Z

crates/superposition/organisation/src/api/handlers.rs

+    mut db_conn: DbConnection,
+) -> superposition::Result<HttpResponse> {
+    let org =
+        db_conn.transaction::<_, superposition::AppError, _>(|transaction_conn| {


Why a transaction for a get request?

Datron · 2024-12-20T08:53:00Z

crates/superposition_types/migrations/2024-12-19-095525_create_organisation_table/up.sql

+    created_by VARCHAR(255) NOT NULL,
+    admin_email VARCHAR(255) NOT NULL,
+    status superposition.org_status NOT NULL DEFAULT 'ACTIVE',
+    contact_details JSONB,


We don't need this column anymore since we flattened it out right?

Datron · 2024-12-20T08:55:44Z

crates/superposition_types/migrations/2024-12-19-095525_create_organisation_table/up.sql

+    sector VARCHAR(100),
+    industry VARCHAR(100),


Should we add this later? A business can span multiple sectors and industries

ShreyBana · 2024-12-20T09:33:59Z

crates/superposition/src/main.rs

@@ -44,6 +45,13 @@ async fn main() -> Result<()> {
    let service_prefix: String =
        get_from_env_unsafe("SERVICE_PREFIX").expect("SERVICE_PREFIX is not set");

+    let options = IdGeneratorOptions::new()
+        .worker_id(1)


Shouldn't we expect worker id in the env?

ShreyBana · 2024-12-20T09:34:33Z

crates/superposition_types/migrations/2024-12-19-095525_create_organisation_table/up.sql

+
+CREATE TABLE IF NOT EXISTS superposition.organisation (
+    id VARCHAR(30) PRIMARY KEY NOT NULL, 
+    country_code VARCHAR(10),


Isn't country code supposed to be 2/3 characters, depending on the format?
Source: https://en.wikipedia.org/wiki/Country_code

Have created a ticket for country code validation to be picked up later.

ShreyBana · 2024-12-20T10:35:37Z

crates/superposition/organisation/src/api/types.rs

+}
+
+#[derive(Deserialize)]
+pub enum SortBy {


nit: SortBy followed by an order, ascending/descending sounds quite odd, as it's an adjective in the place of a noun. It makes sense to sort-by length, size, color etc., but not by an ordering itself, as it can't be the property of a subject. As in this case, an organization can a have size or length, but it can't possibly be ascending or descending by some magnitude.
I know that this term has already been used in the system for this use-case, but since it's going to be present in API contracts(& maybe UI), I feel like we should seriously consider changing it to Order or OrderBy.

ShreyBana · 2024-12-20T10:37:22Z

crates/superposition/organisation/src/api/types.rs

+}
+
+#[derive(Deserialize, Default)]
+pub struct OrganisationFilters {


Can you see if you can use PaginationParams instead of your own type?

these types are redundant, I will remove them

ShreyBana · 2024-12-20T10:46:35Z

@sauraww - If any of the comments will/is taking too much time to resolve, ping me, we can take a call on whether to take it up later.

Datron · 2024-12-20T12:16:31Z

crates/superposition_types/src/database/schema.rs

+diesel::table! {
+    use diesel::sql_types::*;
+    use super::sql_types::OrgStatus;
+
+    organisation (id) {
+        #[max_length = 30]
+        id -> Varchar,
+        #[max_length = 10]
+        country_code -> Nullable<Varchar>,
+        #[max_length = 255]
+        contact_email -> Nullable<Varchar>,
+        #[max_length = 15]
+        contact_phone -> Nullable<Varchar>,
+        #[max_length = 255]
+        created_by -> Varchar,
+        #[max_length = 255]
+        admin_email -> Varchar,
+        status -> OrgStatus,
+        #[max_length = 100]
+        sector -> Nullable<Varchar>,
+        created_at -> Timestamp,
+        updated_at -> Timestamp,
+        #[max_length = 255]
+        updated_by -> Varchar,
+    }
+}
+


@sauraww since this is inside superposition schema, it should look different

pub mod superposition { pub mod sql_types { #[derive(diesel::sql_types::SqlType)] #[diesel(postgres_type(name = "workspace_status", schema = "superposition"))] pub struct WorkspaceStatus; } diesel::table! { use diesel::sql_types::*; use super::sql_types::WorkspaceStatus; superposition.workspaces (organization_id, workspace_name) { organization_id -> Text, organization_name -> Text, workspace_name -> Text, workspace_schema_name -> Text, workspace_status -> WorkspaceStatus, workspace_admin_email -> Text, created_by -> Text, last_modified_by -> Text, last_modified_at -> Timestamp, created_at -> Timestamp, mandatory_dimensions -> Nullable<Array<Text>>, } } }

This is what workspace looks like for the same create statement as yours.
You have to change diesel.toml to reflect this:

[print_schema.superposition] file = "src/superposition_schema.rs" patch_file = "schema.patch"

ayushjain17 · 2024-12-20T12:10:30Z

crates/superposition_types/src/database/models/organisation.rs

+#[cfg_attr(feature = "diesel_derives", diesel(check_for_backend(diesel::pg::Pg)))]
+#[cfg_attr(feature = "diesel_derives", diesel(primary_key(id)))]
+#[cfg_attr(feature = "diesel_derives", diesel(treat_none_as_null = true))]
+#[cfg_attr(feature = "diesel_derives", diesel(table_name = organisation))]


should the table name be organisations ?

ayushjain17 · 2024-12-20T12:11:07Z

crates/superposition_types/src/database/models.rs

@@ -2,3 +2,5 @@ pub mod cac;

 #[cfg(feature = "experimentation")]
 pub mod experimentation;
+


can we please avoid these newlines

ayushjain17 · 2024-12-20T12:20:59Z

crates/superposition_types/migrations/2024-12-19-095525_create_organisation_table/up.sql

+-- up.sql
+CREATE SCHEMA IF NOT EXISTS superposition;
+
+CREATE TYPE superposition.org_status AS ENUM ('ACTIVE', 'INACTIVE', 'PENDINGKYB');


should it be PENDING_KYB ?

ayushjain17 · 2024-12-20T12:24:51Z

crates/superposition/src/organisation/types.rs

+#[derive(Serialize)]
+pub struct OrganisationResponse {
+    id: String,
+    country_code: Option<String>,
+    contact_email: Option<String>,
+    contact_phone: Option<String>,
+    admin_email: String,
+    status: OrgStatus,
+    sector: Option<String>,
+    created_at: chrono::NaiveDateTime,
+    updated_at: chrono::NaiveDateTime,
+    created_by: String,
+    updated_by: String,
+}
+
+impl From<Organisation> for OrganisationResponse {
+    fn from(org: Organisation) -> Self {
+        OrganisationResponse {
+            id: org.id,
+            country_code: org.country_code,
+            contact_email: org.contact_email,
+            contact_phone: org.contact_phone,
+            admin_email: org.admin_email,
+            status: org.status,
+            sector: org.sector,
+            created_at: org.created_at,
+            updated_at: org.updated_at,
+            created_by: org.created_by,
+            updated_by: org.updated_by,
+        }
+    }
+}


why do we need this separately, its same as Organisation type, can we please reuse that
type unification was done to avoid these things 😭

ayushjain17 · 2024-12-20T12:25:32Z

crates/superposition/Cargo.toml

 serde_json = { workspace = true }
 service_utils = { path = "../service_utils" }
 superposition_types = { path = "../superposition_types" }
 toml = { workspace = true }
+idgenerator = "2.0.0"


can we add this also in alphabetical order

ayushjain17 · 2024-12-20T12:30:46Z

crates/superposition/src/organisation/handlers.rs

+            }
+        })?;
+
+    Ok(HttpResponse::Ok().json(OrganisationResponse::from(org)))


if we can remove the OrganisationResponse and directly use Organisation type then we can simplify it here to:

Suggested change

Ok(HttpResponse::Ok().json(OrganisationResponse::from(org)))

Ok(HttpResponse::Ok().json(org))

ayushjain17 · 2024-12-20T12:32:59Z

crates/superposition/src/organisation/types.rs

+    pub country_code: Option<String>,
+    pub contact_email: Option<String>,
+    pub contact_phone: Option<String>,
+    pub created_by: String,


this should not be a request param, this should be taken from the User's username

ayushjain17 · 2024-12-20T12:33:23Z

crates/superposition/src/organisation/handlers.rs

+        country_code: req.country_code.clone(),
+        contact_email: req.contact_email.clone(),
+        contact_phone: req.contact_phone.clone(),
+        created_by: req.created_by.clone(),


this should not be a request param, this should be taken from the User's username

ayushjain17 · 2024-12-20T12:35:01Z

crates/superposition/src/organisation/handlers.rs

+    let DbConnection(mut conn) = db_conn;
+
+    // Generating a numeric ID from IdInstance and prefixing it with `orgid`
+    let numeric_id = IdInstance::next_id();


we planned to keep the org id of fixed length 25 right ?
to allow for some buffer in future?

orgid is 5 characters, wanted to just know that how is IdInstance::next_id() making sure that the length of the string given is of 20 characters exact ?

or are we keeping the id value dynamic and not fixed length, because I was of the impression that we planned to keep the id fixed length

ayushjain17 · 2024-12-20T12:36:00Z

crates/superposition_types/migrations/2024-12-19-095525_create_organisation_table/up.sql

+CREATE TYPE superposition.org_status AS ENUM ('ACTIVE', 'INACTIVE', 'PENDINGKYB');
+
+CREATE TABLE IF NOT EXISTS superposition.organisation (
+    id VARCHAR(30) PRIMARY KEY NOT NULL, 


we planned to keep the org id of fixed length 25 right ?
to allow for some buffer in future?

It's my review

ayushjain17 · 2024-12-20T14:01:56Z

crates/superposition/src/main.rs

@@ -181,6 +192,11 @@ async fn main() -> Result<()> {
                            AppExecutionScopeMiddlewareFactory::new(AppScope::EXPERIMENTATION),
                        ),
                    )
+                    .service(
+                        scope("/organisation")
+                            .wrap(AppExecutionScopeMiddlewareFactory::new(AppScope::SUPERPOSITION))


this has to be cleaned up later

ayushjain17 · 2024-12-20T14:08:21Z

flake.nix

@@ -29,7 +29,7 @@
        devShells.default = pkgs.mkShell {
          inputsFrom = [
            self'.devShells.rust
-            self'.devShells.haskell
+            # self'.devShells.haskell


please uncomment this later on

sauraww requested a review from a team as a code owner December 20, 2024 08:35

sauraww force-pushed the organisation-management branch from c27e5b4 to e9347bb Compare December 20, 2024 08:48

Datron requested changes Dec 20, 2024

View reviewed changes

sauraww force-pushed the organisation-management branch 2 times, most recently from 778257c to ba5455a Compare December 20, 2024 10:40

ShreyBana reviewed Dec 20, 2024

View reviewed changes

sauraww force-pushed the organisation-management branch from ba5455a to 098dde5 Compare December 20, 2024 11:55

ShreyBana previously approved these changes Dec 20, 2024

View reviewed changes

Datron force-pushed the organisation-management branch from 098dde5 to 0fe5e68 Compare December 20, 2024 12:13

Datron reviewed Dec 20, 2024

View reviewed changes

Datron approved these changes Dec 20, 2024

View reviewed changes

ayushjain17 reviewed Dec 20, 2024

View reviewed changes

ShreyBana self-requested a review December 20, 2024 12:41

ayushjain17 force-pushed the organisation-management branch from 0fe5e68 to c7cafec Compare December 20, 2024 14:00

ayushjain17 reviewed Dec 20, 2024

View reviewed changes

feat: added schema and crl apis for organisation

2a09fb1

Datron force-pushed the organisation-management branch from c7cafec to 2a09fb1 Compare December 20, 2024 14:06

ayushjain17 reviewed Dec 20, 2024

View reviewed changes

ayushjain17 approved these changes Dec 20, 2024

View reviewed changes

ayushjain17 merged commit 061b47f into saas Dec 20, 2024
4 checks passed

ayushjain17 deleted the organisation-management branch December 20, 2024 14:21

Datron pushed a commit that referenced this pull request Jan 2, 2025

feat: added schema and crl apis for organisation (#322)

9270000

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added schema and crl apis for organisation #322

added schema and crl apis for organisation #322

sauraww commented Dec 20, 2024

Datron Dec 20, 2024

Datron Dec 20, 2024

Datron Dec 20, 2024

Datron Dec 20, 2024

Datron Dec 20, 2024

Datron Dec 20, 2024

ShreyBana Dec 20, 2024

ShreyBana Dec 20, 2024

ShreyBana Dec 20, 2024

ShreyBana Dec 20, 2024

ShreyBana Dec 20, 2024

sauraww Dec 20, 2024

ShreyBana commented Dec 20, 2024

Datron Dec 20, 2024

ayushjain17 Dec 20, 2024

ayushjain17 Dec 20, 2024

ayushjain17 Dec 20, 2024

ayushjain17 Dec 20, 2024

ayushjain17 Dec 20, 2024

ayushjain17 Dec 20, 2024

ayushjain17 Dec 20, 2024

ayushjain17 Dec 20, 2024

ayushjain17 Dec 20, 2024

ayushjain17 Dec 20, 2024

ayushjain17 Dec 20, 2024

ayushjain17 Dec 20, 2024

		@@ -2,3 +2,5 @@ pub mod cac;

		#[cfg(feature = "experimentation")]
		pub mod experimentation;

	Ok(HttpResponse::Ok().json(OrganisationResponse::from(org)))
	Ok(HttpResponse::Ok().json(org))

added schema and crl apis for organisation #322

added schema and crl apis for organisation #322

Conversation

sauraww commented Dec 20, 2024

Problem

Solution

Environment variable changes

Pre-deployment activity

Post-deployment activity

API changes

Possible Issues in the future

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShreyBana commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment