Code review

[gsnider2195]

Development environment setup (cookie issues?)

invoke build fails with default docker-compose.base.yml:

Building Nautobot with Python 3.7...
Running docker-compose command "build"
parsing /home/gary/github/nautobot-plugin-floor-plan/development/docker-compose.base.yml: yaml: unmarshal errors:
  line 25: mapping key "<<" already defined at line 24

After commenting out line 25 <<: *nautobot-base, the build works but then the image name doesn't work:

The local development image builds as nautobot_floor_plan-nautobot but the dockerfile uses nautobot-floor-plan/nautobot:${NAUTOBOT_VER}-py${PYTHON_VER}

Same error seen with the docs service in docker-compose.dev.yml

Commented out line in postgres docker compose file

Documentation

Hard-coded link on the index page of the docs makes for a confusing dev experience when trying to read the docs in the dev docs container. This is due to the index of the docs reusing the main projects README.md

mkdocs.yml copyright is "Copyright © The Authors". Is this correct?

Minimum version

Dockerfile is defaulting to nautobot version 1.0.1. Should this be the version when locations were introduced?

__init__.py min_version is set to 1.4.0

.cookiecutter.json min_nautobot_version is set to 1.5.0

pyproject.toml is set to nautobot ^1.4.0

Should these be aligned?

UX

Using the mousewheel to zoom the floor plan also scrolls the page. (#16)

A full screen view of the floor plan would be really nice. Trying to pan a large floor plan vertically with the floor plan constrained to the bottom of the view is frustrating.

500 error when navigating to /plugins/floor-plan/floor-plan-tiles/

Duplicated edit buttons on floor plan detail view

Adding a tile and then resizing the floor plan to be too small to fit that tile does not delete the tile. Should the tiles be deleted to prevent invalid data from being left in the database?

Filters

RackFilterExtension.nautobot_floor_plan_floor_plan is a ModelChoiceFilter. Is it possible to make this a ModelMultipleChoiceFilter to match the majority of filters in core and the FloorPlanTileFilterSet.floor_plan filter?

Feature request? (nitpick)

The floor plan detail page shows rows for Tile Width (Relative Units) and Tile Depth (Relative Units). I think this could do a better job of portraying the actual meaning if the unit of measurement could be selected and then was used in these rows. ex: if I set units to parsecs the row would say Tile Width (parsecs): 1000

Misc

Commented out line in models.py

And here

Might be helpful to add help text to the FloorPlanTile x_origin and y_origin explaining that (1,1) is top left

FloorPlanTile.bounds would be a good candidate for a namedtuple

[glennmatthews]

Development environment setup (cookie issues?)

All of these are bugs in the base cookiecutter that have since been resolved upstream. We need to pull in those fixes.

Documentation

Hard-coded link on the index page of the docs makes for a confusing dev experience when trying to read the docs in the dev docs container. This is due to the index of the docs reusing the main projects README.md

mkdocs.yml copyright is "Copyright © The Authors". Is this correct?

These appear to be consistent with our other apps and with the latest version of the cookiecutter.

Minimum version

Dockerfile is defaulting to nautobot version 1.0.1. Should this be the version when locations were introduced?

Yeah, it should default to 1.4.0 probably. Not critical since tasks.py sets nautobot_ver: latest but would be good to fix.

__init__.py min_version is set to 1.4.0

.cookiecutter.json min_nautobot_version is set to 1.5.0

pyproject.toml is set to nautobot ^1.4.0

Should these be aligned?

cookiecutter.json just reflects the initial values this was baked with (initially I don't think we were covering 1.4.x), but it wouldn't hurt to update it to 1.4.0 to be consistent.

UX

A full screen view of the floor plan would be really nice. Trying to pan a large floor plan vertically with the floor plan constrained to the bottom of the view is frustrating.

Agreed, would be a good enhancement.

500 error when navigating to /plugins/floor-plan/floor-plan-tiles/

Duplicated edit buttons on floor plan detail view

You're talking about these two?

We could remove the lower "edit" button but I'm not sure I see it as a problem personally.

Adding a tile and then resizing the floor plan to be too small to fit that tile does not delete the tile. Should the tiles be deleted to prevent invalid data from being left in the database?

Definitely.

Filters

RackFilterExtension.nautobot_floor_plan_floor_plan is a ModelChoiceFilter. Is it possible to make this a ModelMultipleChoiceFilter to match the majority of filters in core and the FloorPlanTileFilterSet.floor_plan filter?

Possibly could, though this is really for internal use as an API filter for a DynamicModelChoiceField rather than a user-facing feature (a user would normally just filter Racks by Location)s), not by location__floor_plan), so I'm not sure it's particularly important.

Feature request? (nitpick)

The floor plan detail page shows rows for Tile Width (Relative Units) and Tile Depth (Relative Units). I think this could do a better job of portraying the actual meaning if the unit of measurement could be selected and then was used in these rows. ex: if I set units to parsecs the row would say Tile Width (parsecs): 1000

Not a bad idea.

Misc

Commented out line in models.py

Intentional, for documentation purposes

And here

Same, documentational.

Might be helpful to add help text to the FloorPlanTile x_origin and y_origin explaining that (1,1) is top left

:+1:

FloorPlanTile.bounds would be a good candidate for a namedtuple

:+1:

[itdependsnetworks]

mkdocs.yml copyright is "Copyright © The Authors". Is this correct?

Yes, confirmed with Cristian

[jvanderaa]

@glennmatthews @itdependsnetworks @gsnider2195 is this something that we can close out or that still needs to be addressed?

[glennmatthews]

I believe most of these are still unaddressed.

[itdependsnetworks]

nothing from my side is required.