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

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

Merged
merged 3 commits into from Apr 4, 2023

Conversation

nickovs
Copy link
Contributor

@nickovs nickovs commented Mar 24, 2023

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 nickovs changed the title Fix #268. Allow table actions when data is as dicts Fix #268. Allow table actions when data is provided as dicts Mar 24, 2023
@nickovs
Copy link
Contributor Author

nickovs commented Apr 3, 2023

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

@PanderMusubi
Copy link
Collaborator

Only greyli has merge rights

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2023

Codecov Report

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

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

📣 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
@@             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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@greyli
Copy link
Member

greyli commented Apr 4, 2023

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

@nickovs
Copy link
Contributor Author

nickovs commented Apr 4, 2023

@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 greyli merged commit 22374ff into helloflask:master Apr 4, 2023
14 checks passed
@greyli
Copy link
Member

greyli commented Apr 4, 2023

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

Merged, thanks!

@nickovs
Copy link
Contributor Author

nickovs commented Apr 4, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

render_table without SQL-Alchemy and Action URLs
4 participants