Skip to content

Feat/ef migration final#271

Open
EinarSalbouvet wants to merge 11 commits intofeat/dotnet-apifrom
feat/ef-migration-final
Open

Feat/ef migration final#271
EinarSalbouvet wants to merge 11 commits intofeat/dotnet-apifrom
feat/ef-migration-final

Conversation

@EinarSalbouvet
Copy link
Collaborator

No description provided.

Copy link
Contributor

@rupakkatwal rupakkatwal left a comment

Choose a reason for hiding this comment

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

Nice work!
Some comments

Copy link
Contributor

Choose a reason for hiding this comment

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

can also be added those properties in outgoingdto?

Copy link
Contributor

Choose a reason for hiding this comment

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

In ProjectRole Update: With Update first, the foreach loop iterates over entities that might be deleted in the next step.`
Also this update make ProjectRoleRepository UpdateRangeAsync Redundant? same with other like objectives, strategy etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point on the project role, I will look into what update methods can be reduced by calling these extensions

Copy link
Contributor

Choose a reason for hiding this comment

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

Curious,Why Column field is removed from all the entities?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the column attribute determines the name the prop has in the database. It was there before to match the column names we had, but since we are remaking the database we no longer need the column attribute

Copy link
Contributor

@rupakkatwal rupakkatwal left a comment

Choose a reason for hiding this comment

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

nice work!
Some comments and suggeastions.

entity.ProjectId = incommingEntity.ProjectId;
entity.Name = incommingEntity.Name;
entity.Description = incommingEntity.Description;
entity.Rationale = incommingEntity.Rationale;
Copy link
Contributor

Choose a reason for hiding this comment

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

also add
entity.IconColor = incommingEntity.IconColor;

Comment on lines +212 to +236
public static async Task RemoveOutOfScopeStrategyOptions(this Issue entity, Issue incommingEntity, AppDbContext context)
{
if (!RepositoryUtilities.IsDecisionMovedOutOfStrategyTable(entity, incommingEntity)) return;
var strategyOptionsToBeRemoved = await context.StrategyOptions
.Where(e => e.Option!.Decision!.IssueId == entity.Id)
.ToListAsync();
if (strategyOptionsToBeRemoved.Count != 0)
{
context.StrategyOptions.RemoveRange(strategyOptionsToBeRemoved);
await context.SaveChangesAsync();
}
}

public static async Task RemoveOutOfScopeStrategyOptions(this Decision entity, Decision incommingEntity, AppDbContext context)
{
if (!RepositoryUtilities.IsDecisionMovedOutOfStrategyTable(entity, incommingEntity)) return;
var strategyOptionsToBeRemoved = await context.StrategyOptions
.Where(e => e.Option!.DecisionId == entity.Id)
.ToListAsync();
if (strategyOptionsToBeRemoved.Count != 0)
{
context.StrategyOptions.RemoveRange(strategyOptionsToBeRemoved);
await context.SaveChangesAsync();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public static async Task RemoveOutOfScopeStrategyOptions(this Issue entity, Issue incommingEntity, AppDbContext context)
{
if (!RepositoryUtilities.IsDecisionMovedOutOfStrategyTable(entity, incommingEntity)) return;
var strategyOptionsToBeRemoved = await context.StrategyOptions
.Where(e => e.Option!.Decision!.IssueId == entity.Id)
.ToListAsync();
if (strategyOptionsToBeRemoved.Count != 0)
{
context.StrategyOptions.RemoveRange(strategyOptionsToBeRemoved);
await context.SaveChangesAsync();
}
}
public static async Task RemoveOutOfScopeStrategyOptions(this Decision entity, Decision incommingEntity, AppDbContext context)
{
if (!RepositoryUtilities.IsDecisionMovedOutOfStrategyTable(entity, incommingEntity)) return;
var strategyOptionsToBeRemoved = await context.StrategyOptions
.Where(e => e.Option!.DecisionId == entity.Id)
.ToListAsync();
if (strategyOptionsToBeRemoved.Count != 0)
{
context.StrategyOptions.RemoveRange(strategyOptionsToBeRemoved);
await context.SaveChangesAsync();
}
}
public static async Task RemoveOutOfScopeStrategyOptions(this Issue entity, Issue incommingEntity, AppDbContext context)
{
if (!RepositoryUtilities.IsDecisionMovedOutOfStrategyTable(entity, incommingEntity)) return;
await RemoveStrategyOptions(context, e => e.Option!.Decision!.IssueId == entity.Id);
}
public static async Task RemoveOutOfScopeStrategyOptions(this Decision entity, Decision incommingEntity, AppDbContext context)
{
if (!RepositoryUtilities.IsDecisionMovedOutOfStrategyTable(entity, incommingEntity)) return;
await RemoveStrategyOptions(context, e => e.Option!.DecisionId == entity.Id);
}
private static async Task RemoveStrategyOptions(AppDbContext context, Expression<Func<StrategyOption, bool>> predicate)
{
var strategyOptionsToBeRemoved = await context.StrategyOptions
.Where(predicate)
.ToListAsync();
if (strategyOptionsToBeRemoved.Count != 0)
{
context.StrategyOptions.RemoveRange(strategyOptionsToBeRemoved);
await context.SaveChangesAsync();
}
}


public static Node Update(this Node entity, Node incommingEntity, AppDbContext context)
{
entity.ProjectId = incommingEntity.ProjectId;
Copy link
Contributor

Choose a reason for hiding this comment

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

context is unused here.

// filter out entities not found
if (entities.Count != incomingList.Count)
incomingList = incomingList.Where(e => entities.Select(x => x.Id).Contains(e.Id)).ToList();
await entities.Update(incomingList, DbContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

since the incoming list is pre-filtered to match existing entities, methods RemoveMissingFromCollectionMutate and GetEntitiesToBeAdded inside entity.update effectively does nothing (both lists have the same IDs).

found the same in OutcomeRepository,ProjectRoleRepository, and strategy Repository.

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.

2 participants