Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Area #2015

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove Area #2015

wants to merge 1 commit into from

Conversation

Christdej
Copy link
Contributor

@Christdej Christdej commented Feb 10, 2025

Ready for review checklist:

  • A self-review has been performed
  • All commits run individually
  • Temporary changes have been removed, like console.log, TODO, etc.
  • The PR has been tested locally
  • A test have been written
    • This change doesn't need a new test
  • Relevant issues are linked
  • Remaining work is documented in issues
    • There is no remaining work from this PR that require new issues
  • The changes does not introduce dead code as unused imports, functions etc.

@Christdej Christdej added backend Backend related functionality breaking-change A breaking change which introduces changes to the public APIs frontend Frontend related functionality labels Feb 10, 2025
@Christdej Christdej self-assigned this Feb 10, 2025
Copy link

🔔 Changes in database folder detected 🔔
Do these changes require adding new migrations? 🤔 In that case follow these steps.
If you are uncertain, ask a database admin on the team 😄

@Christdej
Copy link
Contributor Author

Will now give warnings on Return to Home missions if they fail becuase we dont have an Area, awaiting Return Home functionallity to be removed aswell.

@Christdej Christdej force-pushed the removearea branch 3 times, most recently from 1e99cac to 8029def Compare February 10, 2025 13:54
@@ -397,7 +339,6 @@ [.. missionInspectionAreaNames]
Name = missionDefinition.Name,
InspectionFrequency = scheduledMissionQuery.InspectionFrequency,
InstallationCode = scheduledMissionQuery.InstallationCode,
InspectionArea = area.InspectionArea,
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this cause the created MissionRun to always have no inspection area?

Copy link
Contributor

Choose a reason for hiding this comment

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

It either uses a existing missionDefinition or creates a new mission definition here. Since this is the only way new mission definitions are created, and since we don't give the mission definition an inspection area, none of the mission definitions and mission runs will have inspection areas. And since the robots get inspection areas from the first mission they run, they also won't have inspection areas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is true. Im trying to find a good way of adding inspection area while still removing area. Any suggestions?

Comment on lines -439 to -445
if (
missionRun.Robot.CurrentInspectionArea != null
&& !await localizationService.RobotIsOnSameInspectionAreaAsMission(
missionRun.Robot.Id,
missionRun.InspectionArea!.Id
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change should ideally be in a separate commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps it should yeah. Also need to remove the localizationService.RobotIsOnSameInspectionAreaAsMission function perhaps.

Comment on lines -316 to -362
List<Area?> missionAreas;
missionAreas = missionTasks
.Where(t => t.TagId != null)
.Select(t =>
stidService.GetTagArea(t.TagId!, scheduledMissionQuery.InstallationCode).Result
)
.ToList();

var missionInspectionAreaNames = missionAreas
.Where(a => a != null)
.Select(a => a!.InspectionArea.Name)
.Distinct()
.ToList();
if (missionInspectionAreaNames.Count > 1)
{
string joinedMissionInspectionAreaNames = string.Join(
", ",
[.. missionInspectionAreaNames]
);
logger.LogWarning(
"Mission {missionDefinition} has tags on more than one inspection area. The inspection areas are: {joinedMissionInspectionAreaNames}.",
missionDefinition.Name,
joinedMissionInspectionAreaNames
);
}

var sortedAreas = missionAreas
.GroupBy(i => i)
.OrderByDescending(grp => grp.Count())
.Select(grp => grp.Key);
var area = sortedAreas.First();

if (area == null && sortedAreas.Count() > 1)
{
logger.LogWarning(
"Most common area in mission {missionDefinition} is null. Will use second most common area.",
missionDefinition.Name
);
area = sortedAreas.Skip(1).First();
}
if (area == null)
{
logger.LogError(
$"Mission {missionDefinition.Name} doesn't have any tags with valid area."
);
return NotFound($"No area found for mission '{missionDefinition.Name}'.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we perhaps keep this block of code in order to find which inspection area to assign the mission definition? We do not need to have an Area model in the database to do this, as it fetches them from an external API (STID). We do however need the inspection area to have an understanding of STID areas though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Originally we planned InspectionAreas to be a list or graph of Areas. Could we potentially store the references to STID areas as a list in InspectionAreas still, but without storing them as Area objects, for instance by only storing the area name?

@Christdej Christdej force-pushed the removearea branch 2 times, most recently from 4d9b2be to 1c92591 Compare February 13, 2025 09:00
@andchiind
Copy link
Contributor

We could potentially link this issue: #1840, but that will depend on how we solve the inspection area issue

@Christdej
Copy link
Contributor Author

Rest of work to be done documented in:
#2024

@tsundvoll
Copy link
Contributor

tsundvoll commented Feb 17, 2025

Done once per asset (possible with multiple inspection areas per asset) (not per mission)

awaiting

  • using a polygon to define area
  • moving ReturnToHome to ISAR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend related functionality breaking-change A breaking change which introduces changes to the public APIs frontend Frontend related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants