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 multi-deletion of DHCP leases #2792

Merged
merged 5 commits into from
Jun 6, 2024
Merged

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Oct 29, 2023

What does this implement/fix?

IP lease deletion incorrectly utilized parseInt(..., 10) which truncated, e.g., 127.0.0.1 to 127 which was then sent to the API. This PR also adds a further improvement to toast handling where an existing toast can be memorized by the caller to update exactly this one single toast later on with either success or error. This is only relevant for mass-events and can later trivially be extended to other places where this is meaningful.

Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

@DL6ER DL6ER force-pushed the fix/del_dhcp_leases branch from fcb320a to edda66f Compare June 3, 2024 17:21
@DL6ER
Copy link
Member Author

DL6ER commented Jun 3, 2024

Rebased on latest development-v6 and ported missing changes from the recently closed #3034

@@ -178,19 +166,21 @@ function delMsg(ids) {
utils.showAlert(
"success",
"far fa-trash-alt",
"Successfully deleted " + ids.length + " message" + (ids.length > 1 ? "s" : ""),
""
"Successfully deleted message",
Copy link
Member

@rdwebdesign rdwebdesign Jun 3, 2024

Choose a reason for hiding this comment

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

The ID is returned on error. I think we could also show the ID on success.

Suggested change
"Successfully deleted message",
"Successfully deleted message ID: " + id,

Without the ID, if you delete 5 messages there will be 5 toasts with the exactly same text.

Copy link
Member Author

Choose a reason for hiding this comment

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

Two comments: If we use your suggestion, I'd suggest an additional space between : and ". Otherwise, how about putting the ID into the second line (currently null to be disabled)? This is also where we are putting domains/regex, etc. in other toasts.

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with both solutions:

  • space after "ID:" (suggestion updated) or
  • replacing the null parameter with ID: XX.

I just don't want to remove the string "ID:" to avoid confusion between the ID and the count of deleted items.

rdwebdesign
rdwebdesign previously approved these changes Jun 6, 2024
Comment on lines 169 to 170
"Successfully deleted message",
"",
Copy link
Member

Choose a reason for hiding this comment

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

As you said, a second option would be to change the title and also replace the empty message with something like this:

Suggested change
"Successfully deleted message",
"",
"Success",
"Message ID: " + id + " deleted.",

@rdwebdesign rdwebdesign dismissed their stale review June 6, 2024 00:25

Wrong button

scripts/pi-hole/js/messages.js Outdated Show resolved Hide resolved
scripts/pi-hole/js/messages.js Outdated Show resolved Hide resolved
@DL6ER DL6ER requested a review from rdwebdesign June 6, 2024 07:01
@DL6ER DL6ER merged commit 4402d56 into development-v6 Jun 6, 2024
8 checks passed
@DL6ER DL6ER deleted the fix/del_dhcp_leases branch June 6, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants