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

Allow new ticket statuses configuration in settings #1157

Merged
merged 14 commits into from
Feb 7, 2024

Conversation

samsplunks
Copy link
Contributor

@samsplunks samsplunks commented Feb 2, 2024

This pull request allows customization for additional ticket statuses in django-helpdesk.

New statuses appear in the public and private ticket views in the "Respond to this ticket" > "New Status" section.

The newly configured statuses can be added to an HELPDESK_TICKET_OPEN_STATUSES tuple to comply with the OPEN_STATUS behavior.

As we are in a perfectionists with deadlines framework mindset and to be complete, there are two insignificant imperfections to this PR:

  • the dynamic generation of radio buttons introduces a "»" at the end of the status list
  • the sort_string() function in views/staff.py+L1803 will not consider customized statuses

Comment on lines 114 to 119
{% with status_choice_strfmt_s=status_choice.0|stringformat:"s" %}
{% if ticket.status == 1 and status_choice_strfmt_s in "2" %}{% else %}
{% if ticket.status == 2 and status_choice_strfmt_s in "1" %}{% else %}
{% if ticket.status == 3 and status_choice_strfmt_s not in "2,3,4" %}{% else %}
{% if ticket.status == 4 and status_choice_strfmt_s not in "2,4" %}{% else %}
{% if ticket.status == 5 and status_choice_strfmt_s not in "2,5" %}{% else %}
Copy link
Member

Choose a reason for hiding this comment

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

Nice refactoring already! Maybe creating a custom template tag could help simplifying this big if.

We would have something like :

Suggested change
{% with status_choice_strfmt_s=status_choice.0|stringformat:"s" %}
{% if ticket.status == 1 and status_choice_strfmt_s in "2" %}{% else %}
{% if ticket.status == 2 and status_choice_strfmt_s in "1" %}{% else %}
{% if ticket.status == 3 and status_choice_strfmt_s not in "2,3,4" %}{% else %}
{% if ticket.status == 4 and status_choice_strfmt_s not in "2,4" %}{% else %}
{% if ticket.status == 5 and status_choice_strfmt_s not in "2,5" %}{% else %}
{% should_display_status ticket.status status_choice as should_display_status_button %}
{% if should_display_status_button %}

For the implementation of the template tag, I imagine something like this :

@register.simple_tag
def should_display_status(ticket_status, status_choice):
    # Create a mapping between current status and possible next statuses
    mapping = {
        1: [2],
        2: [1],
        3: [2, 3, 4],
        4: [2, 4],
        5: [2, 5],
    }
    return status_choice in mapping[ticket_status]

Maybe, we could add a third parameter in order to reuse it in the public view ticket which seem to have lightly different mapping rules.

@samsplunks
Copy link
Contributor Author

samsplunks commented Feb 5, 2024

I see what you mean, I'm refactoring a static mess with a dynamic one.

Putting code in a templatetag would scatter the logic.
I was surprised the forms were not linked to the model through a ModelForm where the status choices logic would be better implemented for form rendering.

Refactoring all this is a big effort for another task but I read what is on your mind.

I would propose adding a STATUS_CHOICES_FLOW setting structured like your mapping to render the radio boxes easily.
This way, users of the project are also in control of the follow-ups of their newly introduced statuses.

@samsplunks
Copy link
Contributor Author

I'm good, you may review the PR.

Thanks!

@timthelion
Copy link
Collaborator

Looks like some nice cleanup work you've done there :)

@samsplunks
Copy link
Contributor Author

samsplunks commented Feb 5, 2024

Thanks a lot Timothy and thanks Benjamin for suggesting the idea!

@Benbb96
Copy link
Member

Benbb96 commented Feb 6, 2024

Nice work @samsplunks !

I completely agree with you with the fact that it would be better handled with ModelForm. But that would mean even more refactoring so I think it's good for now 🙂

@samsplunks
Copy link
Contributor Author

Copy that @Benbb96, thanks for all the support!

@uhurusurfa uhurusurfa merged commit f379bbe into django-helpdesk:main Feb 7, 2024
8 checks passed
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.

None yet

4 participants