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

Admin UI 1675 #1691

Merged
merged 13 commits into from Nov 25, 2023
Merged

Admin UI 1675 #1691

merged 13 commits into from Nov 25, 2023

Conversation

RobTilton
Copy link
Contributor

Problem

#1675 Admin UI Bug

Solution

Modification to add_success_message, Adding tracking of IMPORT_TYPE_DELETE and IMPORT_TYPE_SKIP.
Modifications where made to a number of Test to insure expected Results where up to date.

Acceptance Criteria
The Test I have added is test_import_for_deletion line 143 of test_admin_integration

What the Message looks like now:
image

I have NOT added the changes to the documentation.

1 Failed Test, Import Legacy Book, Failure due to incorrect string return.

Copy link
Contributor

@matthewhegarty matthewhegarty left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

I suggest some minor changes. First this should go into the release-4 branch (not main). This is because we are changing a public facing message which other implementations may rely on.

The best way to do this from your Admin_UI_1675 branch

# take a backup of existing branch
git branch Admin_UI_1675-bak

git merge release-4

This will give you some conflicts (i.e. in the tests) which you will have to resolve. You can add your new tests to the tests/admin_integration/test_import.py module in release-4.

Then when all your tests pass, you can push to the remote branch and I'll take it from there. Let me know if you need any guidance with this.

Also, please install pre-commit for formatting purposes (see docs).

If you could also add an entry to changelog.rst that would be appreciated.

import_export/admin.py Outdated Show resolved Hide resolved
@RobTilton
Copy link
Contributor Author

RobTilton commented Nov 24, 2023

Regarding
Git merge release-4
Not something we can merge.
I'm not sure how I am to merge a branch from your repo, into a branch on my fork.

Never mind, I found the commands needed.

@matthewhegarty matthewhegarty changed the base branch from main to release-4 November 24, 2023 19:34
@matthewhegarty
Copy link
Contributor

Thanks Rob - I have changed the base branch on the PR to release-4 and we now have your changes, but we're missing the tests.

@RobTilton
Copy link
Contributor Author

working tests now, seems to be String related again, should be done within the next hour. If nothing comes up.

Copy link
Contributor

@matthewhegarty matthewhegarty left a comment

Choose a reason for hiding this comment

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

One change required for failing tests. I will add this fix now. You will need to do a git pull to get my changes.

requirements/test.txt Outdated Show resolved Hide resolved
tests/core/tests/admin_integration/test_import.py Outdated Show resolved Hide resolved
@matthewhegarty matthewhegarty merged commit b9b90f1 into django-import-export:release-4 Nov 25, 2023
5 checks passed
@matthewhegarty matthewhegarty linked an issue Nov 27, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin UI does not show message for deleted objects
2 participants