Sanction check base #803
Sanction check base #803
Conversation
apognu
force-pushed
the
feat/sanctions-in-iterations
branch
4 times, most recently
from
84797bb to
1177382
Compare
apognu
force-pushed
the
feat/sanctions-in-iterations
branch
4 times, most recently
from
862e4eb to
b121202
Compare
Pascal-Delange
force-pushed
the
feat/sanctions-in-iterations
branch
from
b121202 to
04f6206
Compare
There was a problem hiding this comment.
thanks !
It's a bit massive so I didn't read every line in depth, I don't think I have major comments so far (a few minor comments in the PR body)
We can continue with this as a feature branch or merge it and take it up from there, I don't have a strong opinion on either solution really
| "github.com/cockroachdb/errors" | ||
| ) | ||
|
|
||
| func (repo *MarbleDbRepository) GetSanctionCheckConfig(ctx context.Context, exec Executor, |
There was a problem hiding this comment.
question: since the model of scenario iterations contains a *SanctionCheckConfig, and we get the sanction check config by scenario iteration id, are we perhaps better served with a repo method that returns a *SanctionCheckConfig (or a []SanctionCheckConfig) rather than a SanctionCheckConfig with need to check on the error type for "not found" ?
usecases/sanction_check_usecase.go
Outdated
| errors.Wrap(models.NotFoundError, "this sanction is not pending review") | ||
| } | ||
|
|
||
| inboxes, err := uc.inboxRepository.ListInboxes(ctx, uc.executorFactory.NewExecutor(), decision[0].OrganizationId, nil, false) |
There was a problem hiding this comment.
have a look at (i *InboxReader) ListInboxes(...), maybe we can reuse that here (the permission logic for "do I have access to this inbox" should already been handled in it)
There was a problem hiding this comment.
Does that mean we can remove the check below this line (ReadOrUpdateCase) because the call to ListInboxes will already check if the user has access to the inbox, and that is the only permission we are interested in here (if someone has access to an inbox, does that mean they are allowed to update the cases in that inbox?)?
There was a problem hiding this comment.
Yes that is the idea.
Also permissions are a bit more complex on cases/inboxes than on other entities in the app because there is a concept of membership in inboxes (with inbox user roles). This logic is already present in the InboxReader class (and it's one of the more unit-tested parts of the codebase).
I'll propose a change in a PR.
There was a problem hiding this comment.
Actually sorry, my answer was a bit off. Using it will not allow to remove any lines of code that you already wrote - however it will avoid us having to add some more to take into account inbox permissions.
| @@ -217,6 +247,16 @@ func EvalTestRunScenario(ctx context.Context, | |||
| return models.ScenarioExecution{}, err | |||
| } | |||
|
|
|||
| scc, err := repositories.EvalSanctionCheckConfigRepository.GetSanctionCheckConfig(ctx, exec, testRunIteration.Id) | |||
There was a problem hiding this comment.
note for later: we said that we perhaps don't want to call the opensanctions api again in the context of a test run (to check with arnaud)
apognu
changed the title
… for upsert, better handling of NULL vs. not provided.
… tests around HTTP retrieval of sanction checks.
apognu
force-pushed
the
feat/sanctions-in-iterations
branch
from
b4f4779 to
89d35fb
Compare
…hed with other status than "no hit"
No description provided.