Closed Bug 1252982 Opened 4 years ago Closed 4 years ago

Clean up redundant settings code for authentication setting options

Categories

(Firefox for iOS :: General, defect)

Other
iOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sleroux, Assigned: maurya1985, Mentored)

Details

(Whiteboard: [good first bug][lang=swift])

In AuthenticationSettingsViewController, we have 3 settings options which essentially do the same thing - display a passcode confirmation screen [1]. We can DRY this up by creating a Setting subclass for presenting a PasscodeConfirmViewController. Along with this, I would also like to remove the class functions for creating the view controllers and instead build the appropriate view controller using the confirmAction parameter [2]. This way we can store the confirmAction with each setting and not have to worry about calling a special method for each view controller.

[1]https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/AuthenticationManager/AuthenticationSettingsViewController.swift#L25

[2] https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/AuthenticationManager/PasscodeConfirmViewController.swift#L38
Hi Stephan, can you assign this to me?
Assignee: nobody → maurya1985
Assigned it to myself. Let me know if anyone's already on it!
It's all yours!
Status: NEW → ASSIGNED
Hey Maurya - thanks for taking this! I was on PTO last week but I'm back now if you have any questions about the bug or implementation details.
Stephan, I wasn't totally sure about my understanding. Is this along the lines of your suggestions for DRYing up the code? https://pastebin.mozilla.org/8867280

Note that I'm initializing the PasscodeViewController object within the respective settings' initializations. I think there might be a better way of doing this?
Flags: needinfo?(sleroux)
Hey Maurya,

I apologize - I'm not sure if this bug is needed anymore after some work I did recently to add some Touch ID functionality. I'm not sure if there is value in DRY'ing up the code further at this point now that each setting has a single line. If you're interested in taking on another bug, you can check out https://bugzilla.mozilla.org/show_bug.cgi?id=1262133. Otherwise there are a bunch of others we've marked here: http://www.joshmatthews.net/bugsahoy/?mobileios=1&simple=1.
Flags: needinfo?(sleroux)
Stephan,

Sure, no problem. I'll look at other bugs. Btw, it'd be helpful if you could still briefly review https://pastebin.mozilla.org/8867280 . I was trying to understand the code and your inputs would help!

Also, are there other bugs which involve refactoring? Most bugs from the links you shared seem to need further discussions to arrive at a consensus on the feature.
Flags: needinfo?(sleroux)
Hey Maurya,

>  Btw, it'd be helpful if you could still briefly review https://pastebin.mozilla.org/8867280 .

The code looks good and was the approach I was talking about above. Super small nit: Since enabled is optoinal and Setting's constructor accepts an optional you don't need to do the optional unwrapping if check - you can just pass in the enabled? param.

> Also, are there other bugs which involve refactoring?

I don't think we have many bugs for just refactoring work. Most of the refactoring that gets done so far tends to be side product of feature development work. A new bug that we triaged this morning might be interesting to check out: https://bugzilla.mozilla.org/show_bug.cgi?id=1263624. Don't think it requires much external input.
Flags: needinfo?(sleroux)
Stephan, thanks a lot for the review! Have been new to Swift, so this helps :)
Stephan, should we mark this bug as "RESOLVED WONTFIX" ?
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(sleroux)
Resolution: --- → FIXED
Might make more sense to make it RESOLVED INVALID but it's already been resolved so we can keep it as RESOLVED FIXED :D
Flags: needinfo?(sleroux)
You need to log in before you can comment on or make changes to this bug.