feat(authN): Add to all DB entries 'Modified_by'... (#81) #230

michalkrzyz · 2024-09-17T12:27:28Z

Move 'CreatedAt', 'DeletedAt' and 'UpdatedAt' to common entity.Info struct
Add 'CreatedBy' and 'UpdatedBy' to common entity.Info struct

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

What type of PR is this? (check all applicable)

Related Tickets & Documents

Related Issue # (issue)
Closes # (issue)
Fixes # (issue)

Remove if not applicable

Added tests?

👍 yes
🙅 no, because they aren't needed
🙋 no, because I need help
Separate ticket for tests # (issue/pr)

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added to documentation?

📜 README.md
🤝 Documentation pages updated
🙅 no documentation needed
(if applicable) generated OpenAPI docs for CRD changes

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

Move 'CreatedAt', 'DeletedAt' and 'UpdatedAt' to common entity.Info struct Add 'CreatedBy' and 'UpdatedBy' to common entity.Info struct Signed-off-by: Michal Krzyz <[email protected]>

internal/entity/common.go

Add metadata for user Add tests

MR2011 · 2024-09-30T14:17:57Z

internal/e2e/metadata_test.go

+
+var ()
+
+func createUser(port string) {


It would be nice if we use this function in the user_query_test.go too. I would remove the expect checks from the helper function here and only add it to the test that calls this helper function so that its not hidden when a test runs

I am not sure we should remove expect checks from helper function/test framework function. Usually common test functions are checking for invalid operation, raising assertions and stopping the test execution.

ahh yes. Now I get it. You are right, It is not good idea to hide expects in test for user create, so I would like to keep this function (createUser with expects), but it will use another function which is named queryCreateUser (without expects).

MR2011 · 2024-09-30T14:19:43Z

internal/e2e/metadata_test.go

+			Expect(err).Should(BeNil())
+			Expect(createdAt).Should(BeTemporally("~", time.Now().UTC(), 3*time.Second))
+
+			Expect(*user.Metadata.UpdatedBy).To(BeEmpty())


Do we really check for BeEmpty()? in the UpdatedBy field?

I wanted to highlight that UpdatedBy is not assigned when the object is created. In some solutions of such problems we see that modified/updated time is assigned by created time at creation of an object (eg. Linux File)

Add metadata for activity, IssueVariant and IssueRepository

Add metadata for component

Add metadata for componentInstance, componentVersion

Add metadata for evidence

Issue Metadata rename to IssueMetadata, because now metadata means data related to creation time, creation user, update time and update user Add Metadata for Issue

Add metadata for IssueMatch

Add Metadata for IssueMatchChange, Service and SupportGroup

drochow · 2024-10-08T06:27:58Z

internal/api/graphql/graph/schema/activity.graphqls

@@ -23,6 +24,7 @@ type ActivityConnection implements Connection {
 type ActivityEdge implements Edge {
    node: Activity!
    cursor: String
+	metadata: Metadata


why intended differently?

--> looks like 8 spaces instead of 4

drochow · 2024-10-08T06:30:12Z

internal/database/mariadb/init/schema.sql

@@ -12,8 +12,10 @@ create table if not exists Component
    component_name       varchar(256)                          not null,
    component_type       varchar(256)                          not null,
    component_created_at timestamp default current_timestamp() not null,
+    component_created_by varchar(256)                          null,


Oh, well this needs to be a foreign key to a User I guess!

@MR2011 @dorneanu WDYT?

I still think User is misleading here. A component can be as well created by a scanner, right?

Yes, it should be foreign key to a User object. Users have a type; for scanners it's TechnicalUserType, so I don't think it's misleading

drochow · 2024-10-08T06:31:16Z

internal/app/activity/activity_handler.go

@@ -132,6 +132,7 @@ func (a *activityHandler) ListActivities(filter *entity.ActivityFilter, options
 }

 func (a *activityHandler) CreateActivity(activity *entity.Activity) (*entity.Activity, error) {
+	activity.CreatedBy = "Creator"


I guess that's a placeholder. Based on the comment I made we need to decide if that should really be a freetext field or rather a foreign key reference to a User

drochow · 2024-10-08T06:32:01Z

internal/api/graphql/graph/model/models.go

+		CreatedAt: &createdAt,
+		CreatedBy: &em.CreatedBy,


As commented this might need to be a sub-resolver for users?!

dorneanu · 2024-10-08T07:18:37Z

internal/app/activity/activity_handler.go

@@ -152,6 +153,7 @@ func (a *activityHandler) CreateActivity(activity *entity.Activity) (*entity.Act
 }

 func (a *activityHandler) UpdateActivity(activity *entity.Activity) (*entity.Activity, error) {
+	activity.UpdatedBy = "Updater"


This is also a placeholder.

MR2011 · 2024-10-08T07:22:22Z

internal/api/graphql/graph/queryCollection/componentInstance/directRelations.graphql

Most queries don't include the createdBy and updatedBy fields. Do we want to add them?

MR2011 · 2024-10-08T07:23:46Z

internal/api/graphql/graph/schema/issue_variant.graphqls

@@ -10,8 +10,7 @@ type IssueVariant implements Node {
    issueRepository: IssueRepository
    issueId: String
    issue: Issue
-    created_at: DateTime
-    updated_at: DateTime
+	metadata: Metadata


wrong indentation

MR2011 · 2024-10-08T07:24:31Z

internal/app/component/component_handler.go

@@ -105,6 +105,7 @@ func (cs *componentHandler) ListComponents(filter *entity.ComponentFilter, optio
 }

 func (cs *componentHandler) CreateComponent(component *entity.Component) (*entity.Component, error) {
+	component.CreatedBy = "Creator"


Another placeholder

MR2011 · 2024-10-08T07:24:39Z

internal/app/component/component_handler.go

@@ -139,6 +140,7 @@ func (cs *componentHandler) CreateComponent(component *entity.Component) (*entit
 }

 func (cs *componentHandler) UpdateComponent(component *entity.Component) (*entity.Component, error) {
+	component.UpdatedBy = "Updater"


Another placeholder

MR2011 · 2024-10-08T07:24:49Z

internal/app/component_instance/component_instance_handler.go

@@ -111,6 +111,7 @@ func (ci *componentInstanceHandler) ListComponentInstances(filter *entity.Compon
 }

 func (ci *componentInstanceHandler) CreateComponentInstance(componentInstance *entity.ComponentInstance) (*entity.ComponentInstance, error) {
+	componentInstance.CreatedBy = "Creator"


Another placeholder

MR2011 · 2024-10-08T07:24:57Z

internal/app/component_instance/component_instance_handler.go

@@ -131,6 +132,7 @@ func (ci *componentInstanceHandler) CreateComponentInstance(componentInstance *e
 }

 func (ci *componentInstanceHandler) UpdateComponentInstance(componentInstance *entity.ComponentInstance) (*entity.ComponentInstance, error) {
+	componentInstance.UpdatedBy = "Updater"


Another placeholder

MR2011 · 2024-10-08T07:25:03Z

internal/app/component_version/component_version_handler.go

@@ -109,6 +109,7 @@ func (cv *componentVersionHandler) ListComponentVersions(filter *entity.Componen
 }

 func (cv *componentVersionHandler) CreateComponentVersion(componentVersion *entity.ComponentVersion) (*entity.ComponentVersion, error) {
+	componentVersion.CreatedBy = "Creator"


Another placeholder

MR2011 · 2024-10-08T07:25:09Z

internal/app/component_version/component_version_handler.go

@@ -129,6 +130,7 @@ func (cv *componentVersionHandler) CreateComponentVersion(componentVersion *enti
 }

 func (cv *componentVersionHandler) UpdateComponentVersion(componentVersion *entity.ComponentVersion) (*entity.ComponentVersion, error) {
+	componentVersion.UpdatedBy = "Updater"


Another placeholder

MR2011 · 2024-10-08T07:25:15Z

internal/app/evidence/evidence_handler.go

@@ -104,6 +104,7 @@ func (e *evidenceHandler) ListEvidences(filter *entity.EvidenceFilter, options *
 }

 func (e *evidenceHandler) CreateEvidence(evidence *entity.Evidence) (*entity.Evidence, error) {
+	evidence.CreatedBy = "Creator"


Another placeholder

MR2011 · 2024-10-08T07:25:21Z

internal/app/evidence/evidence_handler.go

@@ -122,6 +123,7 @@ func (e *evidenceHandler) CreateEvidence(evidence *entity.Evidence) (*entity.Evi
 }

 func (e *evidenceHandler) UpdateEvidence(evidence *entity.Evidence) (*entity.Evidence, error) {
+	evidence.UpdatedBy = "Updater"


Another placeholder

MR2011 · 2024-10-08T07:25:27Z

internal/app/issue/issue_handler.go

@@ -160,6 +160,7 @@ func (is *issueHandler) ListIssues(filter *entity.IssueFilter, options *entity.I
 }

 func (is *issueHandler) CreateIssue(issue *entity.Issue) (*entity.Issue, error) {
+	issue.CreatedBy = "Creator"


Another placeholder

MR2011 · 2024-10-08T07:26:49Z

internal/database/mariadb/activity.go

@@ -208,9 +211,11 @@ func (s *SqlDatabase) CreateActivity(activity *entity.Activity) (*entity.Activit

 	query := `
 		INSERT INTO Activity (


aren't we missing the updated_by field? This is also in the other db queries the case

Do we need to insert it during creation of an object? is it possible that updatedBy will be given when the object is created in database?

Refactor and review fixes

Test foreign key

feat(authN): Add to all DB entries 'Modified_by'... (#81)

cb2f9c1

Move 'CreatedAt', 'DeletedAt' and 'UpdatedAt' to common entity.Info struct Add 'CreatedBy' and 'UpdatedBy' to common entity.Info struct Signed-off-by: Michal Krzyz <[email protected]>

michalkrzyz force-pushed the mikrzyz/issue-81 branch from e084f8b to cb2f9c1 Compare September 17, 2024 14:51

drochow reviewed Sep 18, 2024

View reviewed changes

internal/entity/common.go Outdated Show resolved Hide resolved

michalkrzyz force-pushed the mikrzyz/issue-81 branch 5 times, most recently from b4e30a8 to 024bc5b Compare September 25, 2024 12:28

feat(authN): Add to all DB entries 'Modified_by'... (#81)

419e412

Add metadata for user Add tests

michalkrzyz force-pushed the mikrzyz/issue-81 branch 2 times, most recently from 94c222f to 2cdfe8c Compare September 26, 2024 17:39

MR2011 reviewed Sep 30, 2024

View reviewed changes

michalkrzyz added 3 commits October 1, 2024 11:23

feat(authN): Add to all DB entries 'Modified_by'... (#81)

abcd977

Add metadata for activity, IssueVariant and IssueRepository

feat(authN): Add to all DB entries 'Modified_by'... (#81)

d6d5698

Add metadata for component

feat(authN): Add to all DB entries 'Modified_by'... (#81)

b8d5d0a

Add metadata for componentInstance, componentVersion

michalkrzyz force-pushed the mikrzyz/issue-81 branch from 2cdfe8c to b8d5d0a Compare October 1, 2024 16:09

michalkrzyz added 4 commits October 2, 2024 00:41

feat(authN): Add to all DB entries 'Modified_by'... (#81)

98500cb

Add metadata for evidence

feat(authN): Add to all DB entries 'Modified_by'... (#81)

69b5884

Issue Metadata rename to IssueMetadata, because now metadata means data related to creation time, creation user, update time and update user Add Metadata for Issue

feat(authN): Add to all DB entries 'Modified_by'... (#81)

6d15ae2

Add metadata for IssueMatch

feat(authN): Add to all DB entries 'Modified_by'... (#81)

6534388

Add Metadata for IssueMatchChange, Service and SupportGroup

drochow requested changes Oct 8, 2024

View reviewed changes

dorneanu reviewed Oct 8, 2024

View reviewed changes

MR2011 reviewed Oct 8, 2024

View reviewed changes

michalkrzyz added 8 commits October 9, 2024 12:01

feat(authN): Add to all DB entries 'Modified_by'... (#81)

8dd97eb

Refactor and review fixes

feat(authN): Add to all DB entries 'Modified_by'... (#81)

7889f91

Test foreign key

Add Foreign Key

70f0cf5

fix tests

19f162e

Failing

a847d76

Stage

ca50f9c

Trying to get issue in e2e

f4ab534

Final

f4ce3e7

michalkrzyz force-pushed the mikrzyz/issue-81 branch from 353848a to f4ce3e7 Compare October 28, 2024 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(authN): Add to all DB entries 'Modified_by'... (#81) #230

feat(authN): Add to all DB entries 'Modified_by'... (#81) #230

michalkrzyz commented Sep 17, 2024 •

edited

Loading

MR2011 Sep 30, 2024

michalkrzyz Oct 7, 2024

michalkrzyz Oct 7, 2024

MR2011 Sep 30, 2024

michalkrzyz Oct 7, 2024

drochow Oct 8, 2024

drochow Oct 8, 2024

drochow Oct 8, 2024

dorneanu Oct 8, 2024

MR2011 Oct 8, 2024

drochow Oct 8, 2024

drochow Oct 8, 2024

dorneanu Oct 8, 2024

MR2011 Oct 8, 2024

MR2011 Oct 8, 2024

MR2011 Oct 8, 2024

MR2011 Oct 8, 2024

MR2011 Oct 8, 2024

MR2011 Oct 8, 2024

MR2011 Oct 8, 2024

MR2011 Oct 8, 2024

MR2011 Oct 8, 2024

MR2011 Oct 8, 2024

MR2011 Oct 8, 2024

MR2011 Oct 8, 2024

michalkrzyz Oct 9, 2024

		@@ -208,9 +211,11 @@ func (s SqlDatabase) CreateActivity(activity entity.Activity) (*entity.Activit

		query := `
		INSERT INTO Activity (

feat(authN): Add to all DB entries 'Modified_by'... (#81) #230

Are you sure you want to change the base?

feat(authN): Add to all DB entries 'Modified_by'... (#81) #230

Conversation

michalkrzyz commented Sep 17, 2024 • edited Loading

Description

What type of PR is this? (check all applicable)

Related Tickets & Documents

Added tests?

Added to documentation?

Checklist

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

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

michalkrzyz commented Sep 17, 2024 •

edited

Loading