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.

Sign up for GitHub

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

Do not update dimension when it already supports unit to avoid fraction/dimensionless collision #1515

Merged
merged 5 commits into from
Feb 24, 2022
Merged

Do not update dimension when it already supports unit to avoid fraction/dimensionless collision #1515

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
144902f
Do not update dimension when it already supports unit to avoid fracti…
abdelr Feb 23, 2022
4f8b113
Fixing test
abdelr Feb 24, 2022
c5d36eb
Naming some constants
abdelr Feb 24, 2022
2934c1a
Using existing code
abdelr Feb 24, 2022
1c54c5a
Update ColumnMappingPresenter.cs
msevestre Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/OSPSuite.Presentation/Presenters/Importer/ColumnMappingPresenter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,10 @@ public void UpdateDescriptionForModel(MappingDataFormatParameter mappingSource)
//The dimension is the first from the supported dimension which
//has the selected unit.
column.Unit = _mappingParameterEditorPresenter.Unit;
column.Dimension = _columnInfos
.First(x => x.DisplayName == model.MappingName)
.SupportedDimensions
.FirstOrDefault(x => x.HasUnit(column.Unit.SelectedUnit));
if (column.Dimension != null && !column.Dimension.HasUnit(column.Unit.SelectedUnit))
column.Dimension = _columnInfos[model.MappingName]
.SupportedDimensions
.FirstOrDefault(x => x.HasUnit(column.Unit.SelectedUnit));
Copy link
Member

Choose a reason for hiding this comment

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

I see this code over an over . FirstOrDefault(x=>x.HasUnit). It needs to be refactored at some point. I am pointing out here because I am seeing it. Please create an issue

Copy link
Member

Choose a reason for hiding this comment

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

it's always coming from the SupportedDimensions. If we make it a real class as opposed to a list? Or it belongs to the Columninfo itself. Not that Supporteddimension is a List and not a IReadOnlyList which breaks encapsulation

Copy link
Member Author

Choose a reason for hiding this comment

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

#1520

}

if (model.ColumnInfo.IsBase())
Expand Down
6 changes: 6 additions & 0 deletions tests/OSPSuite.HelpersForTests/DomainHelperForSpecs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using OSPSuite.Core.Domain;
Expand Down Expand Up @@ -41,6 +42,11 @@ public static IDimension ConcentrationDimensionForSpecs()
return _concentrationDimension;
}

public static IEnumerable<IDimension> ExtendedDimensionsForSpecs()
{
return new [] { ConcentrationDimensionForSpecs(), Constants.Dimension.NO_DIMENSION, FractionDimensionForSpecs() };
}

public static IDimension LengthDimensionForSpecs()
{
if (_lengthDimension == null)
Expand Down
37 changes: 34 additions & 3 deletions tests/OSPSuite.Presentation.Tests/Importer/Presenters/ColumnMappingPresenterSpecs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ namespace OSPSuite.Presentation.Importer.Presenters
{
public abstract class concern_for_ColumnMappingPresenter : ContextSpecification<ColumnMappingPresenter>
{
protected const int GEOMETRIC_ERROR = 0;
protected const int ARITMETIC_ERROR = 1;
protected IDataFormat _basicFormat;
protected IColumnMappingView _view;
protected IImporter _importer;
Expand Down Expand Up @@ -168,7 +170,7 @@ protected override void Context()
{
base.Context();
A.CallTo(() => _mappingParameterEditorPresenter.Unit).Returns(new UnitDescription(""));
A.CallTo(() => _mappingParameterEditorPresenter.SelectedErrorType).Returns(0);
A.CallTo(() => _mappingParameterEditorPresenter.SelectedErrorType).Returns(GEOMETRIC_ERROR);
Copy link
Member

Choose a reason for hiding this comment

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

I think this should return the enum that we have already defined as opposed to number. I am going to accept this PR but create an issue for this because returning the index of the combo box is not ok

UpdateSettings();
}

Expand Down Expand Up @@ -198,7 +200,7 @@ protected override void Context()
{
base.Context();
A.CallTo(() => _mappingParameterEditorPresenter.Unit).Returns(new UnitDescription(""));
A.CallTo(() => _mappingParameterEditorPresenter.SelectedErrorType).Returns(1);
A.CallTo(() => _mappingParameterEditorPresenter.SelectedErrorType).Returns(ARITMETIC_ERROR);
UpdateSettings();
}

Expand Down Expand Up @@ -335,7 +337,7 @@ protected override void Context()
base.Context();
_mappingSource = _parameters[2] as MappingDataFormatParameter;
A.CallTo(() => _mappingParameterEditorPresenter.Unit).Returns(new UnitDescription("µmol/l"));
A.CallTo(() => _mappingParameterEditorPresenter.SelectedErrorType).Returns(1);
A.CallTo(() => _mappingParameterEditorPresenter.SelectedErrorType).Returns(ARITMETIC_ERROR);
UpdateSettings();
}

Expand Down Expand Up @@ -454,4 +456,33 @@ public void the_unit_and_dimension_are_set_properly(string oldUnitDescription, s
mappedColumn.Dimension.HasUnit(mappedColumn.Unit.SelectedUnit).ShouldBeTrue();
}
}

public class When_unit_is_manually_set_to_fraction_empty : concern_for_ColumnMappingPresenter
{
protected MappingDataFormatParameter _mappingSource;

protected override void Context()
{
base.Context();
var supportedDimensions = _columnInfos["Error"].SupportedDimensions;
supportedDimensions.Clear();
supportedDimensions.AddRange(DomainHelperForSpecs.ExtendedDimensionsForSpecs());
_mappingSource = _parameters[2] as MappingDataFormatParameter;
_mappingSource.MappedColumn.Dimension = supportedDimensions.First(x => x.Name == Constants.Dimension.FRACTION);
A.CallTo(() => _mappingParameterEditorPresenter.Unit).Returns(new UnitDescription(""));
A.CallTo(() => _mappingParameterEditorPresenter.SelectedErrorType).Returns(ARITMETIC_ERROR);
UpdateSettings();
}

protected override void Because()
{
sut.UpdateDescriptionForModel(_mappingSource);
}

[Observation]
public void the_dimension_is_not_changed()
{
_mappingSource.MappedColumn.Dimension.ShouldNotBeNull();
}
}
}