1252982 - Clean up redundant settings code for authentication setting options

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Stephan Leroux [:sleroux]]

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

Comment 1

[Maurya Talisetti]

Hi Stephan, can you assign this to me?

Comment 2

[Maurya Talisetti]

Assigned it to myself. Let me know if anyone's already on it!

Comment 3

[Aaron Train [:aaronmt]]

It's all yours!

Comment 4

[Stephan Leroux [:sleroux]]

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.

Comment 5

[Maurya Talisetti]

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?

Comment 6

[Stephan Leroux [: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.

Comment 7

[Maurya Talisetti]

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.

Comment 8

[Stephan Leroux [: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.

Comment 9

[Maurya Talisetti]

Stephan, thanks a lot for the review! Have been new to Swift, so this helps :)

Comment 10

[Maurya Talisetti]

Stephan, should we mark this bug as "RESOLVED WONTFIX" ?

Comment 11

[Stephan Leroux [:sleroux]]

Might make more sense to make it RESOLVED INVALID but it's already been resolved so we can keep it as RESOLVED FIXED :D