TrangPham / django-admin-confirm

AdminConfirmMixin is a mixin for ModelAdmin that adds confirmations to changes, additions and actions.
Other
128 stars 16 forks source link

Raw field names shown in confirmation screen #28

Closed harris-irfan closed 2 years ago

harris-irfan commented 3 years ago

In the confirmation screen, raw field names are shown instead of the human readable field labels. Ideally, from the perspective of a user, they should be shown the same field labels that they see in the change form.

For example, if there is a model field first_name, Django automatically generates the human readable label as "First Name", i.e. by removing underscores and capitalizing initials. The same should be done in the confirmation screen, but currently the field would be displayed as "first_name" instead of "First Name".

TrangPham commented 3 years ago

Hi Harris,

I'd be happy to review a PR if you'd like to submit one. The relevant code is in admin_confirm/templates/admin/change_confirmation.html

nortigo commented 3 years ago

Hi @TrangPham ,

I've created a PR: https://github.com/TrangPham/django-admin-confirm/pull/31

TrangPham commented 3 years ago

Hi Jonathan,

Thanks for your MR! :D At first glance it looks good, but I will pull it and test locally after work.

On Thu, May 20, 2021 at 11:31 AM Jonathan @.***> wrote:

Hi @TrangPham https://github.com/TrangPham ,

I've created a PR: #31 https://github.com/TrangPham/django-admin-confirm/pull/31

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/TrangPham/django-admin-confirm/issues/28#issuecomment-845364545, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQE5QNBBN4PDV7YBPCDZKTTOVIOVANCNFSM43DIMXPQ .

nortigo commented 3 years ago

Sure, no problem.

nortigo commented 3 years ago

Hi @TrangPham ,

Did you get any chance to review my PR ?

TrangPham commented 3 years ago

I updated the description (line 17 tests/market/models.py) to have a verbose name

    description = models.TextField(
        null=True, blank=True, verbose_name="This Is My Verbose Name For Description"
    )

And can confirm that it works :)

Screen Shot 2021-05-26 at 10 45 37 AM

Thanks for the MR 💯 Would you be able to add a unit test or integration test for this?

I think an integration test would be easiest. You'd want to update one of the models on test project to have a verbose description. You can look at test_should_have_hidden_form as an example of how to load a page and check the page source. You can also check out docs/development_process.md for more info on working in the project.

If you're unable/not available to add the test, I could also branch off your PR and add them. Let me know. :)

nortigo commented 3 years ago

Hi @TrangPham

I followed the development process doc but when I try to run the tests I get this is the error:

django.core.exceptions.ImproperlyConfigured: Cannot import 'market'. Check that 'tests.market.apps.MarketConfig.name' is correct.

If I run make run, the project works.

Any idea why?

TrangPham commented 3 years ago

Sorry for the delayed reply. Which command are you using to run the tests?

The make run command is set to ./tests/manage.py run . So this is using your local env, which should be set to django-admin-confirm-3.8.0.

In the setup.cfg, the pytest settings specify the path to the DJANGO_SETTINGS_MODULE. This must be set incorrectly someone how. I can't look into it at this moment.

I'll try to clone in another folder and follow the dev steps from new to see if I can reproduce the issue later.

But you should be able to follow the suggestion here: https://stackoverflow.com/questions/27938620/python-running-test-improperlyconfigured

nortigo commented 3 years ago

Hello @TrangPham

I've recreated the virtualenv, just in case if I missed something. Everything works fine except for the tests. I'm having hard time understand how to run tests. Also I noticed a migration for market was missing and I fix a typo in the development process doc: https://github.com/TrangPham/django-admin-confirm/pull/31/commits/215ec742a3d00d9deeb0b850d865ad5e0a420b0f