Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Preventing initialize when name & email is not present #319

Merged
merged 13 commits into from
Oct 20, 2017

Conversation

StanleyGoldman
Copy link
Contributor

@StanleyGoldman StanleyGoldman commented Sep 14, 2017

@StanleyGoldman StanleyGoldman changed the title Fixes/initialize project requires name email Adding a popup to initialize name & email Sep 14, 2017
@StanleyGoldman StanleyGoldman force-pushed the fixes/initialize-project-requires-name-email branch from dbe7825 to e09d1cd Compare September 18, 2017 19:14
@StanleyGoldman StanleyGoldman changed the title Adding a popup to initialize name & email Preventing initialize when name & email is not present Sep 19, 2017
@shana shana changed the base branch from master to enhancements/initialize-view September 20, 2017 12:36
@shana
Copy link
Member

shana commented Sep 20, 2017

So functionally this is fine, but it's very user unfriendly. If we're requiring a specific user action, we shouldn't present it in the same way as we do errors, users will think it's an error. We should also provide an easy way to go to the place where the user is expected to fill out the needed information - i.e., if we say "Fill out x in this window", we should have a link or button that sends them to it, or we should specify how to get to it.

This is btw why I would prefer not to do these UI changes until next week, because it's just another piece of temporary UI that we will have to fix :/

@shana shana changed the base branch from enhancements/initialize-view to master October 19, 2017 14:32
@StanleyGoldman
Copy link
Contributor Author

What I don't like about this is the odd repetition of this functionality...

isBusy = true;
string username = null;
string email = null;
GitClient.GetConfig("user.name", GitConfigSource.User).Then((success, value) => {
Logger.Trace("Return success:{0} user.name", success, value);
if (success)
{
username = value;
}
}).Then(GitClient.GetConfig("user.email", GitConfigSource.User).Then((success, value) => {
Logger.Trace("Return success:{0} user.email", success, value);
if (success)
{
email = value;
}
})).FinallyInUI((success, ex) => {
Logger.Trace("Return success:{0} name:{1} email:{2}", success, username, email);
isBusy = false;
isUserDataPresent = success && !String.IsNullOrEmpty(username) && !String.IsNullOrEmpty(email);
errorMessage = isUserDataPresent ? null : NoUserOrEmailError;
Logger.Trace("Finally: {0}", isUserDataPresent);
Redraw();
}).Start();

var user = new User();
GitClient.GetConfig("user.name", GitConfigSource.User)
.Then((success, value) => user.Name = value).Then(
GitClient.GetConfig("user.email", GitConfigSource.User)
.Then((success, value) => user.Email = value))
.FinallyInUI((success, ex) =>
{
if (success && !String.IsNullOrEmpty(user.Name))
{
cachedUser = user;
userDataHasChanged = true;
Redraw();
}
})
.Start();

I contemplated making InitProjectView use UserSettingsView but I didn't come up with anything I liked... What do you think @shana?

@StanleyGoldman
Copy link
Contributor Author

Maybe a combined method in GitClient since it's such a common thing to query for both values?

@StanleyGoldman
Copy link
Contributor Author

Yep, that took care of things nicely.
I'm glad we had this chat...

@shana
Copy link
Member

shana commented Oct 20, 2017

🦆

Copy link
Member

@shana shana left a comment

Choose a reason for hiding this comment

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

Some of the changes to reduce duplicated code in this PR don't go far enough, we still have the same piece of code in three different places. However, they're tangential to the bugfix of this PR, so I'm happy with that getting addressed in another PR.

}
})
.Start();
GitClient.GetConfigUserAndEmail().FinallyInUI((success, ex, strings) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is the same code that RepositoryManager is running to load a user, which still feels duplicated. We also shouldn't be calling GitClient directly from the UI, sooo... feels like we should have a User class with static methods that can load itself, it we don't have a Repository/RepositoryManager to do it for us? And then everyone calls that?

@shana
Copy link
Member

shana commented Oct 20, 2017

Also the UI is janky but then again that's not the main point of this PR (is the screenshot up to date?) so I'm ok with merging this and polishing separately.

@StanleyGoldman StanleyGoldman merged commit 090f950 into master Oct 20, 2017
@StanleyGoldman StanleyGoldman deleted the fixes/initialize-project-requires-name-email branch October 20, 2017 18:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants