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

feat: add ability to add history items to job queue #1778

Merged
merged 14 commits into from
Feb 15, 2024

Conversation

mdziekon
Copy link
Contributor

@mdziekon mdziekon commented Feb 14, 2024

Description

This feature request & PR implements new context menu entry in History view, allowing users to add history entries to Queue. Right now, Mainsail only allows to start reprinting historical items immediately, which in certain cases might not be desired (eg. when user wants to print that file later; user is already printing something but wants to quickly schedule next prints from the History view; user wants to reprint a file, but with a different Spool which has to be changed via Spoolman panel on the Dasboard).

After adding an item to the Queue, a toast is being presented, indicating that something has happened. This was added mostly because adding to queue does not force any screen change (and it shouldn't, as there's no reason to do that, users might want to continue adding other historical entries to the Queue), so there should be some form of feedback.

This PR adds only the "Add to Queue" menu item.

This PR:

  • adds both "Add to Queue" & "Add batch to Queue" menu items.
  • refactors and componentizes "Add batch to Queue" dialog used in File explorer, Status files list & (now) History entries list, making it reusable and self-contained.
  • fixes form validation - previously, even though count field validation was applied and displayed "on input", it would not prevent users from submitting the form, by either pressing "Enter" key on the field, or clicking on the "Add to Queue" button.

"Add batch to Queue" is possible, however would require a bit of refactoring (the "Add batch" modal should be refactored into a separate component, to make it reusable and prevent yet another, third I think, implementation to exist). I might take this up as well, but since the basic functionality already works, I believe this can be picked up in the future (in case if I won't be able to pick this up in this PR - let's not stall feature delivery if it's possible to take the iterative approach).

Related Tickets & Documents

N/A

Mobile & Desktop Screenshots/Recordings

obraz
obraz

Signed-off-by: Michał Dziekoński <michal.dziekonski+github@gmail.com>
Copy link
Contributor

Language file analysis report:

File Missing Keys Unused Keys
en.json 0 4

Signed-off-by: Michał Dziekoński <michal.dziekonski+github@gmail.com>
Signed-off-by: Michał Dziekoński <michal.dziekonski+github@gmail.com>
Signed-off-by: Michał Dziekoński <michal.dziekonski+github@gmail.com>
Signed-off-by: Michał Dziekoński <michal.dziekonski+github@gmail.com>
Signed-off-by: Michał Dziekoński <michal.dziekonski+github@gmail.com>
Signed-off-by: Michał Dziekoński <michal.dziekonski+github@gmail.com>
Signed-off-by: Michał Dziekoński <michal.dziekonski+github@gmail.com>
Copy link
Contributor

Language file analysis report:

File Missing Keys Unused Keys
en.json 0 5

Signed-off-by: Michał Dziekoński <michal.dziekonski+github@gmail.com>
Copy link
Contributor

Language file analysis report:

File Missing Keys Unused Keys
en.json 0 5

1 similar comment
Copy link
Contributor

Language file analysis report:

File Missing Keys Unused Keys
en.json 0 5

Signed-off-by: Michał Dziekoński <michal.dziekonski+github@gmail.com>
@mdziekon mdziekon force-pushed the feat/history-item-add-to-queue branch from c919fab to bf5dd33 Compare February 15, 2024 01:01
Copy link
Contributor

Language file analysis report:

File Missing Keys Unused Keys
en.json 0 5

Signed-off-by: Stefan Dej <meteyou@gmail.com>
Copy link
Contributor

Language file analysis report:

File Missing Keys Unused Keys
en.json 0 4

Signed-off-by: Stefan Dej <meteyou@gmail.com>
Copy link
Contributor

Language file analysis report:

File Missing Keys Unused Keys
en.json 0 4

Signed-off-by: Stefan Dej <meteyou@gmail.com>
Signed-off-by: Stefan Dej <meteyou@gmail.com>
Copy link
Contributor

Language file analysis report:

File Missing Keys Unused Keys
en.json 0 4

@meteyou meteyou changed the title feat: Add ability to add history item to queue feat: add ability to add history items to job queue Feb 15, 2024
@meteyou meteyou merged commit 8e7db0d into mainsail-crew:develop Feb 15, 2024
10 checks passed
@meteyou
Copy link
Member

meteyou commented Feb 15, 2024

@mdziekon thank you very much! I just did some refactoring myself, because we are short before the next release and i did here (#1274) a complete refactoring of the history page and with this changes, I think it will fit better to update it in the other PR.

If you have some questions, why i did some things in your code, feel free to ask here!

@mdziekon
Copy link
Contributor Author

@meteyou thanks for merging this so quickly and including it in the next release :)

As for the refactors, no worries, that's understandable. I already did look through all of the commits and all of the changes make sense to me.

I have one note about the parseInt usage in addBatchToQueueAction (that you've refactored), I'll add it as code review for better visibility. Nothing serious, just a note of clarification, doesn't require any fixing actions or anything like that.

}

async addBatchToQueueAction() {
const array = Array(parseInt(this.input)).fill(this.filename)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@meteyou

Note about the parseInt usage - in my unmodified code, I've added the radix argument on purpose, to force the "natural" parsing of inputs, as in "treat strings as natural number input". Typing something like "0x11" makes parseInt assume it's a hexadecimal, and while it is most likely a super rare case, I usually tend to prevent ambiguity in terms of parsing, that's why I've included it. Not sure if you know about this or not, and I also don't mind this being gone here (as I said, rather rare to see that kind of typing mistake made by the users), but just wanted to clarify in case you didn't know about this.

While this does not need any action, please note that the validation rule above still leverages the radix = 10 argument, so the "wrong" (or rather, unnatural) input is impossible to be submitted. However, I think it's usually good to enforce same style of this function's usage, so if you agree with my take, I would recommend adding the radix back in some future refactoring task. This can also be enforced (autofixed even, I think) with ESLint rule: https://eslint.org/docs/latest/rules/radix (always option required, because as-needed incorrectly assumes that 10 is the default).

Copy link
Member

Choose a reason for hiding this comment

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

ohh thx! i didn't know that! Do you want to add this eslint rule with a PR? I think this would be a good think to "fix" this issues in the complete project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can pick this up, shouldn't be much of a problem :)

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.

2 participants