Error TypeError

Cannot read property 'added' of null
TypeError: Cannot read property 'added' of null
    at Object.exports.default (/app/danger-0.4moim58yi9k.ts:16:44)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

11|         warn("The PlayStoreStrings.po file must be updated any time changes are made to release notes");
12|     }
13| 
14|     // If changes were made to the strings, make sure they follow our guidelines
15|     const modifiedStrings = modifiedFiles.filter((path: string) => path.endsWith('values/strings.xml'))
----------------------------------------------^
16| 
17|     for (let file of modifiedStrings) {        
18|         const stringDiffs = await danger.git.diffForFile(file)
19| 

Error TypeError

Cannot read property 'diff' of null
TypeError: Cannot read property 'diff' of null
    at checkCommitDiffs (/app/danger-0.ezuc6com3fn.ts:43:49)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

38|         if (git === undefined) {
39|             console.log("About to crash due to an error")
40|             console.log("File:", thisFile)
41|             console.log("Danger Object: ", danger)
42| 
---------------------------------------------------^
43|             if (danger !== undefined) {
44|                 console.log("Danger is no longer defined")
45|             }
46|             else {

Generated by 🚫 dangerJS

You can test the changes on this Pull Request by downloading the APK here.

Thanks for the review @AmandaRiu!

As discussed, I've reverted to logic to validate the site using the discovery API in e601f9b. The discovery process will now be initiated only when the user clicks on the Login with site credentials and enters their username + password. Since there is no way to complete the jetpack sign in flow implemented in this PR, without calling the discovery API, I've moved this logic to a separate class called LoginBaseDiscoveryFragment. This only applies to Woo at the moment. I have not modified the existing implementation used by wp-android in LoginSiteAddressFragment.

Keyboard stays visible when navigating to the “Magic Link” screen as well as the new LoginNoJetpackFragment, but there are no text fields in these views.

I was not able to replicate this but I added code to close the keypad before redirecting to the "Magic link" or LoginNoJetpackFragment in aa28af6.

The new LoginNoJetpackFragment was a bit confusing since we already have a screen almost identical to this one. Is this necessary because technically the user isn’t logged into WPcom when they view it?

We need Jetpack to be installed/active/connected in order to support signing in with their site credentials. Only when Jetpack is connected, would we be able to get the jetpack_user_email connected with their username + password. And once the email is fetched, we can redirect them to the "Magic link" flow. I created a new fragment and xml file rather than reuse the existing one, because the existing one is in the SitePickerActivity which has a lot of logic in place to check if WooCommerce exists or if user is logged in etc. that is not relevant to this logic here. Since the layout is similar to what we use in the SitePickerActivity (minus the RecyclerView and some layouts specific to that screen), I just copied the layout relevant to the LoginNoJetpackFragment screen.

It would also be helpful to remove anything that isn’t used by this fragment like the user_info_group and renaming the root from site_picker_root as it leads to a lot of confusion when trying to read the code.

Done in 40b0360

The refresh the app to continue and Try again buttons do the same thing.

Agreed! I removed the Refresh the app to continue in 40b0360 since it was not in the design and since the Try Again button does the same thing.

The Log out in the username message looks like it should do something but it does not.

I forgot to add the logout click functionality before. I added it in 40b0360.

The design specs have two separate messages for when Jetpack is NOT installed and when Jetpack is installed but not connected. But since there is no way to know from the wp.getOptions API if jetpack is installed or installed but not connected, we're currently using only the LEFT side message in the app:

.

The entire design flow can be found in #1482 . PR ready for another round 👍

Thanks for the thorough review @AmandaRiu! I've fixed all the code changes and PR ready for another round 👍

We should definitely port that change to the LoginSiteAddressFragment to avoid issues in the future. Since you’re adding this base class in this PR I think it makes the most sense to port that over in this PR to keep it altogether.

I agree that we should definitely port the LoginBaseDiscooveryFragment changes to the LoginSiteAddressFragment. But I would like to work on this in another PR, if that's ok. This is because there is some discussion around how best to handle the error scenarios in our app:

And based on what we decide, we would be making changes to this class and it would make it easier to implement the changes first and then port things over from LoginSiteAddressFragment. What do you think? I've added a separate task for this in the master issue thread here, so it does not get missed.