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

Meter Drop Down #770

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

We don't put VSC setting in the repository except as needed by everyone (such as the container we are now using). While removing trailing whitespaces is generally nice, it would not be if it did it to some files such as markdown. Since we now run ESLint in VSC you should see warning about this. Thus, I think we should remove this change.

"files.trimTrailingWhitespace": true
}
69 changes: 69 additions & 0 deletions src/client/app/containers/MeterDropdownContainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ import { updateSelectedMeter } from '../actions/admin';
import MeterDropdownComponent from '../components/MeterDropDownComponent';
import { State } from '../types/redux/state';
import { Dispatch } from '../types/redux/actions';
import { unitsCompatibleWithMeters } from '../utils/determineCompatibleUnits';
import { SelectOption } from '../types/items';
import Meter from '../../../../src/server/models/Meter';


/**
* @param {State} state
Expand All @@ -23,4 +27,69 @@ function mapDispatchToProps(dispatch: Dispatch) {
};
}

export function getvisibleMeters(state: State) {
Copy link
Member

Choose a reason for hiding this comment

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

A couple of items on this:

  1. I'm not sure the name of this function is the best. The group dropdown group named their function getGroupCompatibilityForDropdown.
  2. The group team put their function in src/client/app/containers/ChartDataSelectContainer.ts. Whatever is done these two should be similar.
  3. I'm unclear on where this is hooked up to the actual menu. The group team also modified the ChartDataSelectContainer.ts to do this. Maybe I'm missing something and it is hard to be certain since the correct meters don't seem to be chosen.

const visibleMeters = new Set<Meter>();

if (state.currentUser.profile?.role == 'admin') {
// Can see all meters that don't have null for unit
visibleMeters.forEach(meter => { meter.id = Meter.getUnitNotNull })
Copy link
Member

Choose a reason for hiding this comment

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

First, visibleMeters was just created so I think it is an empty set. Thus, doing a forEach would seem to iterate over no items. Second, getUnitNotNull is a database function and not an attribute of Meters. Third, comparing the id to the Meter attribute does not seem correct. Thus, this call does not do what I think you want. The same applies to the else clause.

Since all meters are in Redux state, I think it would be best to get the meter info from that and filter appropriately where you add the meter id for each one desired. I think the group dropdown does that.

}
else {
// regular user or not logged in so only displayable ones
visibleMeters.forEach(meter => { meter.id = Meter.getDisplayable })
}
// meters that can graph
const compatibleMeters = new Set<number>();
// meters that cannot graph.
const incompatibleMeters = new Set<number>();
// {M} means turn M into a set.
const M = new Set<number>();

if (state.graph.selectedUnit == -99) {
// If there is no graphic unit then no meters/groups are displayed and you can display all meters.
// Also, if not admin, then meters not displayable are not viewable.
// admin can see all except if unit is null (not included in ones gotten above).
visibleMeters.forEach(meter => {
compatibleMeters.add(meter.displayable);
Copy link
Member

Choose a reason for hiding this comment

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

The checks above should have already set visibleMeters to what is appropriate for admin or non-admin users. As such, you can set compatible meters to visibleMeters and get the correct result.

Note that you seem to be adding only the displayable attribute of each meter to coompatibleMeters and not the id.

})
}
// If displayable is false then only admin.
else {
state.graph.selectedMeters.forEach(meter => {//for each visibleMeters M
Copy link
Member

Choose a reason for hiding this comment

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

The design document specified that the loop was over the visible meters where each iteration called the meter M.

OED prefers that comments be on the line above the code it applies to.

M.add(meter)
Copy link
Member

Choose a reason for hiding this comment

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

I'm unclear what the M.add is supposed to do.

const newUnits = unitsCompatibleWithMeters(M)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why you call this newUnits when the design document specified units as the name. While changing the name may not impact the result, it is nice to keep the code in line with that document unless there is a reason to change it. If you had a reason then please let me know.

if (newUnits.has(state.meters.byMeterID[meter].defaultGraphicUnit))//graphicUnit is in units
Copy link
Member

Choose a reason for hiding this comment

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

This should be checking if the current graphic unit is in the set. This is checking the default graphic unit for a specific meter. They are not the same.

{
// The compatible units of the meter have graphic unit so can graph (case 1)
compatibleMeters.add(meter);
}
else {
// Case 2
incompatibleMeters.add(meter);
}
})
}
//Ready to display meters in menu. Note you display the identifier and not the id.
//For each compatibleUnit C add name C.identifier to meter menu in alphabetically sorted order in regular font for case 1
//ForAdd each incompatibleUnit I add name I.identifier to meter menu in alphabetically sorted order as grayed out and not selectable for case 2
const finalMeter: SelectOption[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

The way this was done was changed (by me) in groups and it should be adapted for use here. This code has some issues in what it is doing so that is another reason to change it.

visibleMeters.forEach(meter => {
if (compatibleMeters.has(meter.unitId)) {
finalMeter.push({
label: state.meters.byMeterID[meter.identifier].name,
value: meter.name,
isDisabled: false
} as SelectOption)
} else if (incompatibleMeters.has(meter.unitId)) {
finalMeter.push({
label: state.groups.byGroupID[meter.identifier].name,
value: meter.name,
isDisabled: true
} as SelectOption)
}
})

return _.sortBy(_.values(finalMeter), 'label');
}

export default connect(mapStateToProps, mapDispatchToProps)(MeterDropdownComponent);