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

Validate presence of project on Staging::Workflow. #10603

Merged
merged 1 commit into from Jan 11, 2021

Conversation

bjoernalbers
Copy link
Contributor

This fixes #7175.

I've added one validation with corresponding tests.

danidoni
danidoni previously approved these changes Jan 7, 2021
@danidoni danidoni dismissed their stale review January 7, 2021 16:14

Ouch, minitests are failing...

@@ -20,6 +20,11 @@
staging_workflow
end

describe 'validations' do
it { is_expected.to validate_presence_of(:managers_group) }
it { is_expected.to validate_presence_of(:project) }
Copy link
Member

Choose a reason for hiding this comment

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

We usually don't test the (presence of) validations. Only if they contain logic.

Copy link
Contributor

@vpereira vpereira Jan 8, 2021

Choose a reason for hiding this comment

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

hm, i like the tests to make sure that the validation exists and wasn't disabled (i.e someone disable it to debug something else, and forget to enable it back) or removed by mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's not possible to create a staging workflow w/o associated project (via the UI) there might be no other way to test the validation.
Plus these tests are quite cheap, no need to hit the db either.

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #10603 (8ac8c76) into master (cb66088) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10603   +/-   ##
=======================================
  Coverage   91.05%   91.05%           
=======================================
  Files         583      583           
  Lines       21481    21481           
=======================================
  Hits        19559    19559           
  Misses       1922     1922           

@dmarcoux
Copy link
Contributor

dmarcoux commented Jan 7, 2021

I wonder if we would need a data migration in addition to the validation. There might be staging workflows without a valid project. I didn't check...

@hennevogel
Copy link
Member

Indeed there are, but they are not accessible anymore anyway as we route through the project. I don't think we need to do anything about them in this PR.

@vpereira
Copy link
Contributor

Indeed there are, but they are not accessible anymore anyway as we route through the project. I don't think we need to do anything about them in this PR.

In production:

 > Staging::Workflow.where(project_id: nil).first
 => 7

@vpereira
Copy link
Contributor

Thank you for your contribution @bjoernalbers!

@vpereira vpereira merged commit b2a51c0 into openSUSE:master Jan 11, 2021
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.

Missing validation for project in Staging:Workflow
5 participants