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

#2534 UID Mapping #2567

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

vaclavbasniar
Copy link
Contributor

Fixes

This pull request fixes issue #2534

Description

  • added modal
  • added storage
  • updated copy and show on server functionality
  • removed connected server from list of servers in context menus
  • unit tests

Type of change

  • Bugfix
  • New feature (non-breaking change which adds functionality)
  • Enhancement of existing functionality
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Impacted Areas in Application

  • Frontend
  • API
  • WITSML
  • Desktop
  • Other (please describe)

Checklist:

Communication

  • I have made corresponding changes to the documentation
  • PR affects application security

Code quality

  • I have self-reviewed my code
  • No new warnings are generated

Test coverage

  • New code is covered by passing tests

Further comments

Copy link
Contributor

@eliasbruvik eliasbruvik left a comment

Choose a reason for hiding this comment

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

Sorry for taking a while. Started on a review, but not finished yet. Anyways, added some comments on things that can be improved, and some questions to help me understand the logic.

Src/WitsmlExplorer.Api/Models/UidMapping.cs Show resolved Hide resolved
Src/WitsmlExplorer.Api/Services/UidMappingService.cs Outdated Show resolved Hide resolved
Src/WitsmlExplorer.Api/Services/UidMappingService.cs Outdated Show resolved Hide resolved
Src/WitsmlExplorer.Frontend/services/uidMappingService.tsx Outdated Show resolved Hide resolved
Src/WitsmlExplorer.Frontend/services/uidMappingService.tsx Outdated Show resolved Hide resolved
Src/WitsmlExplorer.Api/Services/UidMappingService.cs Outdated Show resolved Hide resolved
@vaclavbasniar vaclavbasniar force-pushed the features/2534_wellbore_uid_mapping branch from 9f9a206 to 96fd79d Compare October 24, 2024 09:04
- refactoring
- removed unused usings
- added missing objects mappings for show on server
- added missing validation
- changed complex key to serialized JSON (LiteDB doesn't support complex objects as key)
- updated tests
};

const mappings = await UidMappingService.queryUidMapping(dbQuery);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't getTargetWellboreID also be used here? Not tested, but something like

const sourceWellboreName = toCopy[0].wellboreName;

  const { targetWellId, targetWellboreId } = await getTargetWellboreID({
    sourceServerId: sourceServer.id,
    sourceWellId: sourceWellUid,
    sourceWellboreId: sourceWellboreUid,
    targetServerId: targetServer.id
  });

  let wellbore: Wellbore;
  try {
    wellbore = await WellboreService.getWellbore(
      targetWellId,
      targetWellboreId,
      null,
      targetServer
    );
  } catch {
    return; // Cancel the operation if unable to authorize to the target server.
  }

  const targetWellboreRef: WellboreReference = {
    wellUid: targetWellId,
    wellboreUid: targetWellboreId,
    wellName: wellbore?.wellName ?? sourceWellName,
    wellboreName: wellbore?.name ?? sourceWellboreName
  };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll check that again. However, I guess this would probably need a little bit more refactoring.

Copy link
Contributor

@eliasbruvik eliasbruvik Oct 28, 2024

Choose a reason for hiding this comment

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

If I map a wellbore to a wellbore on another server, and then delete the target wellbore, the mapping will exist but not the target wellbore. If I then try to use the copyToServer-functionality it asks me to create the target wellbore as expected. However, it fails with this message:
image

I would expect either of these (maybe this should be discussed?):

  • Any mapping that exists for a wellbore that is deleted should also be removed. If this is chosen, we need to take into account what should happen if the wellbore is deleted from the query view or another application (when we don't know that it has been deleted).
  • CopyToServer will create the wellbore with the mapped uid, and then copy the object over (without any errors).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's a good point, that hasn't been discussed. I think, the first solution is correct. We should probably discuss this in weekly meeting.

- refactoring
- removed unused usings
- added missing objects mappings for show on server
- added missing validation
- changed complex key to serialized JSON (LiteDB doesn't support complex objects as key)
- updated tests
# Conflicts:
#	Src/WitsmlExplorer.Frontend/components/ContextMenus/FluidContextMenu.tsx
#	Src/WitsmlExplorer.Frontend/components/ContextMenus/LogCurveInfoContextMenu.tsx
#	Src/WitsmlExplorer.Frontend/components/ContextMenus/LogsContextMenu.tsx
#	Src/WitsmlExplorer.Frontend/components/ContextMenus/ObjectMenuItems.tsx
#	Src/WitsmlExplorer.Frontend/components/ContextMenus/ObjectsSidebarContextMenu.tsx
#	Src/WitsmlExplorer.Frontend/components/ContextMenus/TrajectoryStationContextMenu.tsx
#	Src/WitsmlExplorer.Frontend/components/ContextMenus/TubularComponentContextMenu.tsx
#	Src/WitsmlExplorer.Frontend/components/ContextMenus/WbGeometrySectionContextMenu.tsx
#	Src/WitsmlExplorer.Frontend/components/ContextMenus/WellContextMenu.tsx
#	Src/WitsmlExplorer.Frontend/components/ContextMenus/WellboreContextMenu.tsx
* bug fixes
* automatic removal of UID mapping on well/wellbore delete
* automatic cleanup of UID mappings pointing to deleted well/wellbores on copy to
* unit tests update
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