feat(authN) - Add udpated_by for delete operations #417
Conversation
michalkrzyz
force-pushed
the
mikrzyz/issue-358
branch
4 times, most recently
from
98e0706 to
eda02c8
Compare
michalkrzyz
force-pushed
the
mikrzyz/issue-358
branch
3 times, most recently
from
1f5b22b to
e4c14f7
Compare
michalkrzyz
requested review from
drochow,
MR2011 and
dorneanu
as code owners
internal/entity/common.go
Outdated
| type StateFilterType int | ||
|
|
||
| const ( | ||
| Active StateFilterType = 0 |
There was a problem hiding this comment.
@reviewers - wouldn't it be better to use string value instead of int for that? Please help me with your opinion.
There was a problem hiding this comment.
Int is okay. I suggest following this enum pattern:
type StateFilterType int
const (
Active StateFilterType = iota
Deleted
All
)
var StateFilterTypeMap = map[StateFilterType]string{
Active: "active",
Deleted: "deleted",
All: "all",
}
func (sft StateFilterType) String() string {
return StateFilterTypeMap[sft]
)
There was a problem hiding this comment.
@michalkrzyz @MR2011 What about using strings like this ? Looks like a nice solution IMHO.
There was a problem hiding this comment.
That's also an option. We should align however on one way to implement enums instead of having multiple different approaches in the codebase
| @@ -30,6 +30,7 @@ type ActivityEdge implements Edge { | |||
| input ActivityFilter { | |||
| serviceCcrn: [String] | |||
| status: [ActivityStatusValues] | |||
| state: Int | |||
There was a problem hiding this comment.
Please create a enum for the API layer.
All filter options are lists. In this case it might not be so useful, but if we add more states it could be possible that we want to filter for multiple states
There was a problem hiding this comment.
So maybe 'All' does not make a sense here, We could have list with State which is Active or Deleted (for all both)?
Anyway why do we want to make an enum here? it will need an extra conversion to convert model enum to entity enum.
As an example of not enum values in similar case there is a user type which is int value in the model.
| @@ -259,20 +259,22 @@ func (s *SqlDatabase) UpdateActivity(activity *entity.Activity) error { | |||
| return err | |||
| } | |||
|
|
|||
| func (s *SqlDatabase) DeleteActivity(id int64) error { | |||
| func (s *SqlDatabase) DeleteActivity(id int64, userId int64) error { | |||
| l := logrus.WithFields(logrus.Fields{ | |||
There was a problem hiding this comment.
Should we add a check on the db layer that the userId can't be 0 (Unknown user)?
@drochow @dorneanu @copyonwrite
There was a problem hiding this comment.
If we want to do sanity checks for user ids, would they be more elaborate than just checking for a zero userId? What about 'historical' users which are not 'active' anymore and similar scenarios?
Saying that, as a rule of thump everything concerning sanity checks for data should be enforced IN the database, not on the db layer.
michalkrzyz
force-pushed
the
mikrzyz/issue-358
branch
2 times, most recently
from
c44743b to
4fa44a9
Compare
… 0 (unknown user)
michalkrzyz
force-pushed
the
mikrzyz/issue-358
branch
from
a73af40 to
8f4dcff
Compare
Description
Add updated_by update for delete operation. This way deleted item can be checked for user who deleted an item (under circumstances that the item was not changed/updated after deletion).
Add possibility to filter through deleted items, active and all (deleted and active). Default is active.
Fix seeding test data in fixture.go for db tests
What type of PR is this? (check all applicable)
Related Tickets & Documents
Added tests?
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?
Checklist