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

tickets views #1158

Merged
merged 23 commits into from
Feb 8, 2025
Merged

tickets views #1158

merged 23 commits into from
Feb 8, 2025

Conversation

ssteeltm
Copy link
Contributor

Kanban view
Compact view

Kanban view
Compact view
@ssteeltm ssteeltm marked this pull request as draft January 29, 2025 14:03
@ssteeltm ssteeltm marked this pull request as ready for review January 29, 2025 14:42
sync last changes on days  27 and 28
@ssteeltm ssteeltm marked this pull request as draft January 29, 2025 17:28
@ssteeltm ssteeltm marked this pull request as ready for review January 29, 2025 18:58
@ssteeltm ssteeltm marked this pull request as draft January 29, 2025 19:05
@ssteeltm ssteeltm marked this pull request as ready for review January 29, 2025 19:36
tickets.php Show resolved Hide resolved
tickets.php Outdated Show resolved Hide resolved
tickets_kanban.php Outdated Show resolved Hide resolved
tickets_kanban.php Outdated Show resolved Hide resolved
@wrongecho
Copy link
Collaborator

Hey!

This is some really nice work here, well done!! I've left a couple comments for you to look over.

Generally speaking, I think we should exclude 'Closed' being a state on the board. I'm fine with resolved, but our stance with a ticket being closed is that you don't get to re-open it afterwards. Currently, the kanban bypasses the resolved -> closed flow and also lets you re-open tickets.
It probably also makes sense to hide the Kanban option when you are just viewing closed tickets (tickets.php?status=Closed) as you can't do anything with them.

Also, I'm thinking we should remove the ability to move columns around and rename ticket_status_kanban to something like ticket_status_order so it's set in the admin settings (like we do in custom_links). That way the admin determines the flow (New > Open > Hold > Custom > Resolved) rather than any random tech. We can use the order elsewhere too.

Thoughts? :)

@ssteeltm
Copy link
Contributor Author

ssteeltm commented Jan 31, 2025

Hey!

This is some really nice work here, well done!! I've left a couple comments for you to look over.

Generally speaking, I think we should exclude 'Closed' being a state on the board. I'm fine with resolved, but our stance with a ticket being closed is that you don't get to re-open it afterwards. Currently, the kanban bypasses the resolved -> closed flow and also lets you re-open tickets. It probably also makes sense to hide the Kanban option when you are just viewing closed tickets (tickets.php?status=Closed) as you can't do anything with them.

Also, I'm thinking we should remove the ability to move columns around and rename ticket_status_kanban to something like ticket_status_order so it's set in the admin settings (like we do in custom_links). That way the admin determines the flow (New > Open > Hold > Custom > Resolved) rather than any random tech. We can use the order elsewhere too.

Thoughts? :)

I agree with you I did changes now to keep resolved rules that you mentioned, so Resolved will remain on columns
About the moving columns, liked ur ideia, but how about doing permissions on this? so only admin can move this columns

@ssteeltm
Copy link
Contributor Author

ssteeltm commented Jan 31, 2025

"t probably also makes sense to hide the Kanban option when you are just viewing closed tickets (tickets.php?status=Closed) as you can't do anything with them."

About this, makes all sense, will update to behave like that

Done

db.sql Outdated Show resolved Hide resolved
db.sql Outdated Show resolved Hide resolved

<script src="/plugins/dragula/dragula.min.js"></script>

<script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

slight nickpick, but can we move this into js/ and call it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jhonny said somewhere that it was to go there on plugins... please tell me where to go and I can change

Copy link
Collaborator

Choose a reason for hiding this comment

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

This bit is fine now that you've moved it to the plugins folder.
<script src="/plugins/dragula/dragula.min.js"></script>

--

Your custom JS (below) needs moving to its own .js file and then including like above.

<script>
    $(document).ready(function() {
        // Initialize Dragula for the Kanban board
        var boardDrake = dragula([
            document.querySelector('#kanban-board')
        ], {
            moves: function(el, container, handle) {
                return handle.classList.contains('panel-title');
            }
        });
[...]

And ideally same for your custom css further up the page.

Copy link
Contributor Author

@ssteeltm ssteeltm Feb 4, 2025

Choose a reason for hiding this comment

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

hey @wrongecho script moved to /js
about css, where can we store the file? I guess we'll have to create a css folder, to put it there

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ohhh, CSS used to have its own folder but we moved it all under vendor/plugins. It can go in with plugins/dragula/, we don't need a dedicated folder just for it - just somewhere to keep it organised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wrongecho @johnnyq This css is related to the page tickets_kanban.php and not dragula itself. Still place it under dragula folder?

@wrongecho
Copy link
Collaborator

Hey ssteeltm,

Nice changes, thank you! I'm really excited to get this reeled in.

We still need to make sure all output is sanitized and unfortunately SonarCloud doesn't catch everything. You can use htmlspecialchars (or our own nullable_htmlentities) to prevent XSS injection from asset names like this:
Screenshot 2025-02-03 202247

--

Is there a reason the Kanban doesn't show the ticket number and includes a blank circle at the top? I thought it was for client tags but apparently not?

Screenshot 2025-02-03 203325

--

About the moving columns, liked ur ideia, but how about doing permissions on this? so only admin can move this columns

Potentially? We'll likely still want to add the ability to manage ticket_status_order via the admin settings and use it as an order by elsewhere.. Let's see what @johnny says.

@ssteeltm
Copy link
Contributor Author

ssteeltm commented Feb 3, 2025

Hey! glad ur enjoying this like me!
this blank if for category... here at least on kanban I didnt printed ticket id at all on screen, I want user can double click it so it opens

I'll do the updates

@ssteeltm
Copy link
Contributor Author

ssteeltm commented Feb 4, 2025

I'm listing here the TODO:

  • XSS Prevention - nullable_htmlentities function
  • Kanban Configs: Allow Status Column Ordering Moves (default: checked)
  • Kanban Configs: Allow Ticket Ordering within its Column (default: unchecked, so it order by priority and ticket id)
  • Kanban JS File
  • Kanban CSS File
  • Ticket Config: Choose the Default View [List, Compact, Kanban]
  • Notification: If $config_ticket_client_general_notifications is enabled, we should be sending the contact & any watchers an email to say it's resolved.

@ssteeltm
Copy link
Contributor Author

ssteeltm commented Feb 4, 2025

Guys, about Kanban Configs... do it under the Defaults item? or create new item Called Kanban ? or make it all under Ticket item

guess it fits better here:
image

@wrongecho
Copy link
Collaborator

Probably worth mentioning that our general principle is keeping things simple with good/opinionated defaults. In general we try and avoid making everything confusing and ultra-customisable. Therefore, I'd say we either include the ability to order tickets & move columns, or we don't. Not everything needs to be customisable.

I mentioned somewhere on the forum it'd be nice to add the default ticket view in settings. Defaults will be the best place for that but it might be best we add that separately after this is merged in the interests of getting this pulled in and tested as part of 25.02.

@johnnyq
Copy link
Collaborator

johnnyq commented Feb 4, 2025

Probably worth mentioning that our general principle is keeping things simple with good/opinionated defaults. In general we try and avoid making everything confusing and ultra-customisable. Therefore, I'd say we either include the ability to order tickets & move columns, or we don't. Not everything needs to be customisable.

I mentioned somewhere on the forum it'd be nice to add the default ticket view in settings. Defaults will be the best place for that but it might be best we add that separately after this is merged in the interests of getting this pulled in and tested as part of 25.02.

I do like the default ticket view in the the ticket setting section. I think we should eventually move away from the defaults settings tab and move the entities in their respectable places such as invoice, ticket, quote etc settings. Its more logical.

@ssteeltm
Copy link
Contributor Author

ssteeltm commented Feb 4, 2025

@johnnyq @wrongecho guess its done for first merge. Please review last changes

@johnnyq
Copy link
Collaborator

johnnyq commented Feb 6, 2025

@ssteeltm looks good, give me some time to review everything once more then we'll pull it in

@johnnyq
Copy link
Collaborator

johnnyq commented Feb 8, 2025

Its looking good @ssteeltm im gonna reel it on in and we can make chnages if necessary during the Feb dev phase.

@johnnyq johnnyq merged commit d92b803 into itflow-org:develop Feb 8, 2025
4 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.

3 participants