🚧 Cleanup how we do bulk actions from list views #2873
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2873 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 371 370 -1
Lines 25385 25247 -138
==========================================
- Hits 25385 25247 -138
Continue to review full report at Codecov.
|
| def apply_action_close(cls, tickets): | ||
| return cls.bulk_close(tickets[0].org, tickets)["changed_ids"] | ||
| def apply_action_close(cls, user, tickets): | ||
| cls.bulk_close(tickets[0].org, tickets) |
There was a problem hiding this comment.
the return value of these apply_ methods is never used - adding user to make the signatures consistent
| fields = ("contact", "subject", "body", "opened_on") | ||
| select_related = ("ticketer", "contact") | ||
| default_order = ("-opened_on",) | ||
| bulk_actions = () |
There was a problem hiding this comment.
using bulk_actions to differentiate between these and actions on a SmartCRUDL
rowanseymour
force-pushed
the
list_actions
branch
from
829efe9
to
f6f029d
Compare
| if action_error: | ||
| print(action_error) | ||
|
|
||
| response["Temba-Toast"] = action_error |
There was a problem hiding this comment.
toast messages look weird and aren't used much in RP but the pjax fetch call is setup to use them so just trying to make the error handling consistent on the Python side and we can change how these errors are surfaced to the user later.
temba/utils/views.py
Outdated
|
|
||
| def get_context_data(self, **kwargs): | ||
| context = super().get_context_data(**kwargs) | ||
| context["actions"] = self.bulk_actions |
There was a problem hiding this comment.
should be renamed to bulk_actions.. but avoiding any changes to templates right now whilst the great refresh is underway
rowanseymour
force-pushed
the
list_actions
branch
from
e967413
to
cf82c7c
Compare
| @@ -2415,24 +2415,6 @@ msgstr "" | |||
| msgid "Stopped" | |||
| msgstr "" | |||
|
|
|||
| msgid "Add to Group" | |||
There was a problem hiding this comment.
these were on the action form classes but never surfaced in the templates
| @@ -185,8 +185,6 @@ def apply_action_archive(cls, user, campaigns): | |||
| for campaign in campaigns: | |||
| EventFire.update_campaign_events(campaign) | |||
|
|
|||
| return [each_campaign.pk for each_campaign in campaigns] | |||
There was a problem hiding this comment.
these return values were never used for anything
temba/flows/views.py
Outdated
| if action == "archive": | ||
| ignored = objects.filter(is_archived=False) | ||
| if ignored: | ||
| flow_names = " and ".join([f.name for f in ignored]) |
There was a problem hiding this comment.
Doesn't seem we can do it this way for localization purposes. Maybe needs to be a list below the warning message?
There was a problem hiding this comment.
Yeah - this is just me copying the original text but we can definitely improve this
temba/utils/views.py
Outdated
| """ | ||
| Need to allow posts which are otherwise not allowed on list views | ||
| """ | ||
| return super().dispatch(*args, **kwargs) |
There was a problem hiding this comment.
Can add POST but should disallow the other verbs here. (DELETE, etc..)
temba/utils/views.py
Outdated
| """ | ||
| Need to allow posts which are otherwise not allowed on list views | ||
| """ | ||
| return super().dispatch(*args, **kwargs) |
There was a problem hiding this comment.
Do you need this? Thought the post method below did what you needed here.
temba/utils/views.py
Outdated
| label = form.cleaned_data.get("label") | ||
|
|
||
| # convert objects queryset to one based only on id | ||
| objects = self.model._default_manager.filter(id__in=[o.id for o in objects]) |
There was a problem hiding this comment.
can we filter by org generically here?
There was a problem hiding this comment.
these ids are coming from a queryset which is filtered by org - is the desire to filter by org again just to be doubly sure?
There was a problem hiding this comment.
Ah, because the list is derived from get_queryset? Hadn't noticed that, probably fine. That said, if everything that has actions on it does have an org_id then being extra paranoid doesn't sound like the worst idea in the world.
There was a problem hiding this comment.
Yeah so what you get from the form is the original list view's queryset + id filtering. This then just converts it to a new queryset that only has id filtering. I needed this because things the original queryset has additional filtering by attributes .. attributes which might be changed by the bulk action, and then objects disappear from the queryset
temba/utils/views.py
Outdated
| action_error = ", ".join(e.messages) | ||
| except Exception: | ||
| logger.exception(f"error applying '{action}' to {self.model.__name__} objects") | ||
| action_error = _("Oops, so sorry. Something went wrong!") |
There was a problem hiding this comment.
An error occurred while making your changes. Please try again.
There was a problem hiding this comment.
( another case of me reusing the original text )
Addresses rapidpro#1244. Current setup is pretty weird.. if you want to add new actions you need to add them to a mixin class in
temba.utilswhich needs knowledge of how all the different model classes work, what permissions different actions can throw etc. It returns error like things but they get ignored... or sometimes just throws exceptions