Skip to content

Conversation

@sleepinggenius2
Copy link
Contributor

Fixes: #9361

Makes the replicate_components and adopt_components fields available in the UI to also be available when doing a module bulk import.

@kkthxbye-code
Copy link
Contributor

Thank you for the PR! I'm not a big fan of having that much duplicate code for the clean method.

@jeremystretch is there any clean way to share the clean method between the model form and the CSV form? I guess we could create a mixin with a clean_module method, just not sure where to put it.

The code functioned as expected when testing.

@jeremystretch
Copy link
Member

I don't have a strong preference for an approach, but we definitely can't just replicate the existing code. Maybe a utility function within the dcim.forms module?

@sleepinggenius2
Copy link
Contributor Author

Updated PR to move clean logic to a common class that both forms can inherit from. This works in my testing, but I'm not very familiar with all of this, so please let me know if this isn't a good solution.

@sleepinggenius2
Copy link
Contributor Author

As we're getting closer to the v3.3 release, I just wanted to follow up on this to see if there's anything else I need to do to be able to get this merged.

@sleepinggenius2
Copy link
Contributor Author

@kkthxbye-code or @jeremystretch, as there have been a number of point releases now since this PR was submitted and we continue to get closer to a v3.3 release, I just wanted to follow up again to see if there is anything I still need to do to get this merged. Thank you.

Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

@sleepinggenius2 I think the approach looks good, thanks for the revision. Could you extend the import view test to verify the function of the two new fields please?

@sleepinggenius2
Copy link
Contributor Author

@sleepinggenius2 I think the approach looks good, thanks for the revision. Could you extend the import view test to verify the function of the two new fields please?

I assume that this will need to go in the ModuleTestCase class of netbox/dcim/tests/test_views.py? I'm not particularly familiar with testing in Python, but I think I have enough information to give it a shot.

@sleepinggenius2
Copy link
Contributor Author

@jeremystretch, I requested a review a month ago after I made the changes that you asked for, but it is still showing Changes requested and there has been no movement, so I apologize if there was something I missed in that process.

@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Dec 11, 2022
@sleepinggenius2
Copy link
Contributor Author

@jeremystretch or @kkthxbye-code, what do I need to do to get this merged or at least kept from being closed?

@kkthxbye-code kkthxbye-code removed the pending closure Requires immediate attention to avoid being closed for inactivity label Dec 13, 2022
Copy link
Contributor

@kkthxbye-code kkthxbye-code left a comment

Choose a reason for hiding this comment

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

Sorry this fell through the cracks @sleepinggenius2

I checked out the PR and rebased on the development and everything works exactly as it should, both for CSV import and through the UI for all combinations of replication/adoption. So this looks good to me, can we merge this @jeremystretch ?

# Ensure that interface is created with no module
self.assertIsNone(interface.module)

# Create a module with adopted components
Copy link
Contributor

@kkthxbye-code kkthxbye-code Dec 13, 2022

Choose a reason for hiding this comment

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

One small thing, this testcase actually fails on my PC. It's trying to install in the first modulebay of the device, which is already occupied from the setup. If I change to another bay it passes. Not sure if it's something with my setup or from the rebase.

Testcase that fails:

self.assertEqual(self._get_queryset().count(), initial_count + len(csv_data) - 1)

AssertionError: 3 != 4

https://github.com/netbox-community/netbox/pull/9498/files#diff-fb7b770d4e8b9f98f2376f15c9e9fac16cb773b6d80e760be04c9ad2fd45ccf2R1978

Copy link
Member

Choose a reason for hiding this comment

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

Same. Maybe related to 9ef24d3? In any case, changing it to an empty bay appears to fix the test.

@sleepinggenius2
Copy link
Contributor Author

I appreciate your all's assistance with this.

@jeremystretch
Copy link
Member

Not sure what's going on with CI today. Tests pass locally, though I'm getting failure notifications from GitHub. Nothing's showing up for the PR currently.

Going to go ahead and merge this into develop and deal with any remaining issues there. Thanks for your work on this @sleepinggenius2 and sorry for the delays!

@jeremystretch jeremystretch merged commit b369309 into netbox-community:develop Dec 13, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add replication and association booleans to Module bulk import

3 participants