Skip to content
This repository has been 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 1 commit
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
39 changes: 34 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("You must provide a title!", comment: "Invalid title 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.

Thoughts on rewording this alert? Maybe something kinder like:

Title: "Title Required" or "Issue requires title"
Body: "Please provide a title and try again"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, I like title option 2... and I like the body. Mind updating that, @wayni208?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before I resolve this and because I had made the change before you replied @BasThomas , are we good on the message as I've written it? Here are both:

        guard let titleText = titleText else {
            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
        }

Also

cancelAction_onCancel(
            texts: [titleText, bodyText],
            title: NSLocalizedString("Unsaved Changes", comment: "New Issue - Cancel w/ Unsaved Changes Title"),
            message: NSLocalizedString("Are you sure you want to discard this new issue? Your message will be lost.",
                                       comment: "New Issue - Cancel w/ Unsaved Changes Message")
        )

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good!

return
}

guard let bodyText = bodyText else {
Squawk.showIssueError(message: NSLocalizedString("You must provide an issue description!", comment: "Invalid description when sending new issue"), view: bodyField)
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,20 @@ 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 :)

if titleText == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe combine these 2 ifs with an or ( ||) or all three with a ternary. Something like

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well the false : true is unnecessary, of course. Although you should then flip the result. 😊

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hahahahah! Yes correct on both points. Totally forget the ternary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I can change this around. How should I resubmit? I close and then do a new PR right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wayni208 no need to close. Once you make a new commit to your branch it will update here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There we go! Thanks for the feedback and direction on that function. Such a more elegant way to get it done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome! Was about to say you don't have to reopen every time. 💯

navigationItem.rightBarButtonItem?.isEnabled = false
return
}

if bodyText == nil {
navigationItem.rightBarButtonItem?.isEnabled = false
return
}

navigationItem.rightBarButtonItem?.isEnabled = true
}

// MARK: UITextFieldDelegate

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

// MARK: UITextViewDelegatre
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Spelling - 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()
}

}