Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import page: fix wizard form #7702

Merged
merged 1 commit into from Jan 4, 2021
Merged

Import page: fix wizard form #7702

merged 1 commit into from Jan 4, 2021

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 25, 2020

The initial data is set at the class level,
this means it doesn't survive for the next form.
I'm using the same method the wizard uses to store information per
session.

This doesn't mean the data is always set as default after saving,
it means the default values are going to be set when the user
is in the advanced form.

So, due to that I have moved the default_branch field to the
basic form, this way the form will always have the default branch.

Close #7697

🧙‍♂️

@stsewd stsewd requested a review from a team November 25, 2020 15:15
@stsewd stsewd force-pushed the fix-wizard-form branch 2 times, most recently from bcf2bad to 1b5281d Compare November 25, 2020 16:47
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This seems fine, but I feel like we probably want to think about this UX a bit more. Will users feel they need to fill out the default_branch in the form? Will they often fill it out with the wrong information? We probably need some JS that dynamically adjusts the default branch to a good default, based on the repo.

Or we should just hide it with CSS or something. I just feel like there is a reason that this wasn't on this form historically, and showing it to users feels wrong without a lot more UX around it.

@@ -85,7 +85,7 @@ class ProjectBasicsForm(ProjectForm):

class Meta:
model = Project
fields = ('name', 'repo', 'repo_type')
fields = ('name', 'repo', 'repo_type', 'default_branch')
Copy link
Member

Choose a reason for hiding this comment

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

Are we showing users a blank default_branch field on all manual imports now? This seems..not great, and pretty confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

For users importing manually yeah. We do show the help text that says that it will be master/trunk if it's empty, not sure if that's enough, we could also make it hidden so users who import from the vcs would get the correct value. But we still want users who do a manual import to be able to set this value, so not sure.

Copy link
Member

Choose a reason for hiding this comment

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

The issue here being that building will be broken if they aren't using a standard VCS-derived default branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, we will use master, but everyone is using main, or maybe other names and the first build will fail as the repo doesn't have that branch.

@ericholscher
Copy link
Member

We should probably also fix the docstring here to say main instead of master:

'<code>trunk</code> or <code>master</code>).',
. Perhaps better to also change our code to support both for git repos?

@stsewd
Copy link
Member Author

stsewd commented Dec 7, 2020

We should probably also fix the docstring here to say main instead of master:

'<code>trunk</code> or <code>master</code>).',

. Perhaps better to also change our code to support both for git repos?

we still use master internally, so changing to main isn't correct there. We could also do a data migration and set to master/trunk for old projects explicitly, and default to main for new ones. But I guess users could also still import an old project that has master as default branch.

@stsewd stsewd mentioned this pull request Dec 14, 2020
The initial data is set at the class level,
this means it doesn't survive for the next form.
I'm using the same method the wizard uses to store information per
session.

This doesn't mean the data is always set as default after saving,
it means the default values are going to be set when the user
is in the advanced form.

So, due to that I have moved the default_branch field to the
basic form, this way the form will always have the default branch.
@stsewd stsewd merged commit f5390f3 into master Jan 4, 2021
3 checks passed
@stsewd stsewd deleted the fix-wizard-form branch January 4, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Values from remote repositories aren't being set in the import form
2 participants