Skip to content
This repository was archived by the owner on Sep 20, 2023. It is now read-only.

Fix for "Disallow Empty Bug Reports" Issue #2615 #2676

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix for "Disallow Empty Bug Reports" Issue #2615 #2676

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a2505e1
Fix for "Disallow Empty Bug Reports" Issue #2615
wayni208 Mar 10, 2019
a8137a8
Fix for "Disallow Empty Bug Reports" Issue #2615
wayni208 Mar 10, 2019
bceb428
Fix for "Disallow Empty Bug Reports" Issue #2615
wayni208 Mar 10, 2019
d78d504
Fix for "Disallow Empty Bug Reports" Issue #2615
wayni208 Mar 11, 2019
a43a06f
Fix for "Disallow Empty Bug Reports" Issue #2615
wayni208 Mar 11, 2019
26bdaff
Fix for "Disallow Empty Bug Reports" Issue #2615
wayni208 Mar 11, 2019
3a16297
Fix for "Disallow Empty Bug Reports" Issue #2615
wayni208 Mar 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions Classes/New Issue/NewIssueTableViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ protocol NewIssueTableViewControllerDelegate: class {
func didDismissAfterCreatingIssue(model: IssueDetailsModel)
}

final class NewIssueTableViewController: UITableViewController, UITextFieldDelegate {
final class NewIssueTableViewController: UITableViewController, UITextFieldDelegate, UITextViewDelegate {

weak var delegate: NewIssueTableViewControllerDelegate?

Expand Down Expand Up @@ -103,6 +103,9 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg
titleField.delegate = self
titleField.font = Styles.Text.body.preferredFont

// Setup the description textView to report if it has been edited
bodyField.delegate = self

// Setup markdown input view
bodyField.githawkConfigure(inset: false)
setupInputView()
Expand Down Expand Up @@ -141,7 +144,12 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg
/// Attempts to sends the current forms information to GitHub, on success will redirect the user to the new issue
@objc func onSend() {
guard let titleText = titleText else {
Squawk.showError(message: NSLocalizedString("You must provide a title!", comment: "Invalid title when sending new issue"))
Squawk.showIssueError(message: NSLocalizedString("An issue title is required. Please add a title and try again.", comment: "Invalid title when sending new issue"), view: bodyField)
return
}

guard let bodyText = bodyText else {
Squawk.showIssueError(message: NSLocalizedString("Please provide as much of a detailed description possible and try again.", comment: "Invalid description when sending new issue"), view: bodyField)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I think this might be a bit confusing. Of course a good issue description is welcome, but not required. GitHub itself allows an empty body; I think we shouldn't fight that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about we just throw up an "Are you sure you want to submit this issue with no description?" confirmation before send?

#2615

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BasThomas @Huddie

I've made a few changes and reverted back to the original behavior where it would turn on the submit button once the titleField had changes. I've also added a confirmation dialogue notifying the user they have no description text and asking if they still want to submit or return to then form to add one.

I had to do a funky deal to get the dialogue correct. I split the onSend function in two because of the @objc function call in the accessibility button function and I needed another function to call on if the user decided to go ahead with the submit. I'm pretty certain this could have been done an easier way so please take a look and give me some direction if there is a better way to implement this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest I'm still in favor of allowing an empty description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BasThomas as in not popping an alert before sending an empty description?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exactly!

return
}

Expand All @@ -155,7 +163,7 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg
owner: owner,
repo: repo,
title: titleText,
body: (bodyText ?? "") + signature)
body: bodyText + signature)
) { [weak self] result in
guard let strongSelf = self else { return }
strongSelf.setRightBarItemIdle()
Expand Down Expand Up @@ -205,6 +213,10 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg
bodyField.inputAccessoryView = actions
}

func updateSubmitButtonState() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you like to write a test for this? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do away with that function as I mentioned. But if we don't, I sure will!

Copy link
Contributor Author

@wayni208 wayni208 Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm concerned about having split that onSend function in two and if I need to write a test for it. Also concerned about writing the test. I couldn't see anywhere where we are currently testing this portion of the code. I'm unfamiliar with tests in general so if you could give me some direction on this I would appreciate it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll get back to you on this one once I have a bit more time. Let's leave it for now; it's OK :)

navigationItem.rightBarButtonItem?.isEnabled = ( titleText == nil || bodyText == nil ) ? false : true
Copy link
Collaborator

@Huddie Huddie Mar 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Like @BasThomas mentioned, you actually can just leave it as

navigationItem.rightBarButtonItem?.isEnabled = !(titleText == nil || bodyText == nil)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you did there. 😀

}

// MARK: UITextFieldDelegate

/// Called when the user taps return on the title field, moves their cursor to the body
Expand All @@ -213,11 +225,18 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg
return false
}

// MARK: UITextViewDelegate

/// Called when editing changed on the body field, enable/disable submit button based on title and body
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome you thought about the header docs! If we drop the body requirement though, this should be updated.

func textViewDidChange(_ bodyField: UITextView) {
updateSubmitButtonState()
}

// MARK: Actions

/// Called when editing changed on the title field, enable/disable submit button based on title text
/// Called when editing changed on the title field, enable/disable submit button based on title and body
@IBAction func titleFieldEditingChanged(_ sender: Any) {
navigationItem.rightBarButtonItem?.isEnabled = titleText != nil
updateSubmitButtonState()
}

}
5 changes: 5 additions & 0 deletions Classes/Systems/Squawk+GitHawk.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,9 @@ extension Squawk {
triggerHaptic()
}

static func showIssueError(message: String, view: UIView?) {
Squawk.shared.show(in: view, config: errorConfig(text: message))
triggerHaptic()
}

}