helloflask / bootstrap-flask

Bootstrap 4 & 5 helper for your Flask projects.
https://bootstrap-flask.readthedocs.io
Other
1.1k stars 189 forks source link

Fix #268. Allow table actions when data is provided as dicts #284

Closed nickovs closed 1 year ago

nickovs commented 1 year ago

This PR fixes issue #268 to allow table actions when the table data is not taken from SQLAlchemy.

Currently each action URLs is formed by looking up the row record in SQLAlchemy, using the row's primary key and the provided model class. Not only does this mean that the URL creation fails for records that are not taken from SQLAlchemy (or something with the same API) but it also means that there is an additional database fetch for every action on every row.

The fix here makes the model parameter optional, even when actions are required. If no model is provided then any fields needed for building the URL are taken from the original row data. If a model class is provided then the functionality should be identical to before, complete with the extra database queries. This allows the record used to find URL arguments to be taken from a different model to that of the row data, as long as that table is indexed with the same primary key.

fixes #268

nickovs commented 1 year ago

@greyli @PanderMusubi Any chance someone could approve the workflow, so that the build tests can run, and then look into merging this?

PanderMusubi commented 1 year ago

Only greyli has merge rights

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: +2.98 :tada:

Comparison is base (9b066cb) 97.01% compared to head (aa3dd43) 100.00%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #284 +/- ## =========================================== + Coverage 97.01% 100.00% +2.98% =========================================== Files 1 1 Lines 134 135 +1 Branches 17 16 -1 =========================================== + Hits 130 135 +5 + Misses 1 0 -1 + Partials 3 0 -3 ``` [see 1 file with indirect coverage changes](https://codecov.io/gh/helloflask/bootstrap-flask/pull/284/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=helloflask) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=helloflask). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=helloflask)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

greyli commented 1 year ago

Looks good! Can you add some tests for this fix?

nickovs commented 1 year ago

@greyli I have added a variation of your test_render_table_with_actions() test case to perform all the same tests but with no database and the data passed as a list of dicts instead of Model instances.

greyli commented 1 year ago

I updated tests to reuse the existing test function (https://github.com/helloflask/bootstrap-flask/pull/284/commits/1b850645ccff349e0ffc0706778c6bbbe33fcebb).

Merged, thanks!

nickovs commented 1 year ago

I updated tests to reuse the existing test function (1b85064).

Sounds good.

Merged, thanks!

You are welcome! Thanks for creating this in the first place!