search

django-cms / djangocms-transfer

django CMS Transfer allows you to export and import plugins.
https://www.django-cms.org
Other
19 stars 19 forks source link

Fix error on export when getting filename #33

Closed wfehr closed 4 months ago

wfehr commented 4 months ago

Both cases result in an AttributeError for trying 'None.get_slug()'.

This error was introduced with the changes in #15.

With the changes of this PR the occuring AttributeError is avoided and a fallback filename used if no "page" is present.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 31.18%. Comparing base (7a1a005) to head (437a123).

Files Patch % Lines
djangocms_transfer/forms.py 0.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #33 +/- ## ========================================== - Coverage 31.34% 31.18% -0.16% ========================================== Files 11 11 Lines 402 404 +2 Branches 60 61 +1 ========================================== Hits 126 126 - Misses 276 278 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fsbraun commented 4 months ago

@wfehr Thanks for this PR!

I just had a quick look. Does with also recover gracefully on re-import? Would it be possible to have a test for that?

wfehr commented 4 months ago

@wfehr Thanks for this PR!

I just had a quick look. Does with also recover gracefully on re-import? Would it be possible to have a test for that?

I'm not sure I understand those questions correctly.

As for testing: Besides "migrations" there aren't really tests for anything, which the codecov-bot also "knows". I think for general testing of the project a new issue or something would be better suited.

The changes done here would currently fix an issue introduced with other latest changes.

fsbraun commented 4 months ago

Sorry, if my question was not clear: Have you tested (manually) to reimport the resulting output?

I understand that tests are close to not existing here. Would it be feasible to create a simple test that creates some plugins (including in a static placeholder), exports them and reimports to verify the process works? If that is not feasible now, it might be a great separate PR.

wfehr commented 4 months ago

Sorry, if my question was not clear: Have you tested (manually) to reimport the resulting output?

I understand that tests are close to not existing here. Would it be feasible to create a simple test that creates some plugins (including in a static placeholder), exports them and reimports to verify the process works? If that is not feasible now, it might be a great separate PR.

I tested export/import with this changes, yes. Since this change only affects the filename, the import (if it would be broken so far) wouldn't be affected otherwise since the filename is irrelevant for that.

As for testing: I'll create a new issue for that so this will be done seperately. A new PR can reference that issue then.