diff options
38 files changed, 765 insertions, 474 deletions
diff --git a/app/assets/javascripts/ide/components/new_dropdown/modal.vue b/app/assets/javascripts/ide/components/new_dropdown/modal.vue index 04ecd4ba4e7..c9c4e9e86f8 100644 --- a/app/assets/javascripts/ide/components/new_dropdown/modal.vue +++ b/app/assets/javascripts/ide/components/new_dropdown/modal.vue @@ -51,8 +51,11 @@ export default { return __('Create file'); }, - isCreatingNew() { - return this.entryModal.type !== modalTypes.rename; + isCreatingNewFile() { + return this.entryModal.type === 'blob'; + }, + placeholder() { + return this.isCreatingNewFile ? 'dir/file_name' : 'dir/'; }, }, methods: { @@ -107,9 +110,12 @@ export default { v-model="entryName" type="text" class="form-control qa-full-file-path" - placeholder="/dir/file_name" + :placeholder="placeholder" /> - <ul v-if="isCreatingNew" class="prepend-top-default list-inline qa-template-list"> + <ul + v-if="isCreatingNewFile" + class="file-templates prepend-top-default list-inline qa-template-list" + > <li v-for="(template, index) in templateTypes" :key="index" class="list-inline-item"> <button type="button" diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index b7e9f7c2028..ded084e9b10 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -93,6 +93,7 @@ export default { }, computed: { ...mapGetters([ + 'convertedDisscussionIds', 'getNoteableData', 'nextUnresolvedDiscussionId', 'unresolvedDiscussionsCount', @@ -301,6 +302,10 @@ export default { note: { note: noteText }, }; + if (this.convertedDisscussionIds.includes(this.discussion.id)) { + postData.return_discussion = true; + } + if (this.discussion.for_commit) { postData.note_project_id = this.discussion.project_id; } diff --git a/app/assets/javascripts/notes/components/notes_app.vue b/app/assets/javascripts/notes/components/notes_app.vue index 6d72b72e628..9eb69dd91ae 100644 --- a/app/assets/javascripts/notes/components/notes_app.vue +++ b/app/assets/javascripts/notes/components/notes_app.vue @@ -60,6 +60,7 @@ export default { ...mapGetters([ 'isNotesFetched', 'discussions', + 'convertedDisscussionIds', 'getNotesDataByProp', 'isLoading', 'commentsDisabled', @@ -193,7 +194,9 @@ export default { /> <placeholder-note v-else :key="discussion.id" :note="discussion.notes[0]" /> </template> - <template v-else-if="discussion.individual_note"> + <template + v-else-if="discussion.individual_note && !convertedDisscussionIds.includes(discussion.id)" + > <system-note v-if="discussion.notes[0].system" :key="discussion.id" diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index ff65f14d529..1ae8b9a0686 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -83,12 +83,44 @@ export const updateNote = ({ commit, dispatch }, { endpoint, note }) => dispatch('startTaskList'); }); -export const replyToDiscussion = ({ commit }, { endpoint, data }) => +export const updateOrCreateNotes = ({ commit, state, getters, dispatch }, notes) => { + const { notesById } = getters; + + notes.forEach(note => { + if (notesById[note.id]) { + commit(types.UPDATE_NOTE, note); + } else if (note.type === constants.DISCUSSION_NOTE || note.type === constants.DIFF_NOTE) { + const discussion = utils.findNoteObjectById(state.discussions, note.discussion_id); + + if (discussion) { + commit(types.ADD_NEW_REPLY_TO_DISCUSSION, note); + } else if (note.type === constants.DIFF_NOTE) { + dispatch('fetchDiscussions', { path: state.notesData.discussionsPath }); + } else { + commit(types.ADD_NEW_NOTE, note); + } + } else { + commit(types.ADD_NEW_NOTE, note); + } + }); +}; + +export const replyToDiscussion = ({ commit, state, getters, dispatch }, { endpoint, data }) => service .replyToDiscussion(endpoint, data) .then(res => res.json()) .then(res => { - commit(types.ADD_NEW_REPLY_TO_DISCUSSION, res); + if (res.discussion) { + commit(types.UPDATE_DISCUSSION, res.discussion); + + updateOrCreateNotes({ commit, state, getters, dispatch }, res.discussion.notes); + + dispatch('updateMergeRequestWidget'); + dispatch('startTaskList'); + dispatch('updateResolvableDiscussonsCounts'); + } else { + commit(types.ADD_NEW_REPLY_TO_DISCUSSION, res); + } return res; }); @@ -262,25 +294,7 @@ export const saveNote = ({ commit, dispatch }, noteData) => { const pollSuccessCallBack = (resp, commit, state, getters, dispatch) => { if (resp.notes && resp.notes.length) { - const { notesById } = getters; - - resp.notes.forEach(note => { - if (notesById[note.id]) { - commit(types.UPDATE_NOTE, note); - } else if (note.type === constants.DISCUSSION_NOTE || note.type === constants.DIFF_NOTE) { - const discussion = utils.findNoteObjectById(state.discussions, note.discussion_id); - - if (discussion) { - commit(types.ADD_NEW_REPLY_TO_DISCUSSION, note); - } else if (note.type === constants.DIFF_NOTE) { - dispatch('fetchDiscussions', { path: state.notesData.discussionsPath }); - } else { - commit(types.ADD_NEW_NOTE, note); - } - } else { - commit(types.ADD_NEW_NOTE, note); - } - }); + updateOrCreateNotes({ commit, state, getters, dispatch }, resp.notes); dispatch('startTaskList'); } diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 0ffc0cb2593..5026c13dab5 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -4,6 +4,8 @@ import { collapseSystemNotes } from './collapse_utils'; export const discussions = state => collapseSystemNotes(state.discussions); +export const convertedDisscussionIds = state => state.convertedDisscussionIds; + export const targetNoteHash = state => state.targetNoteHash; export const getNotesData = state => state.notesData; diff --git a/app/assets/javascripts/notes/stores/modules/index.js b/app/assets/javascripts/notes/stores/modules/index.js index 887e6d22b06..6168aeae35d 100644 --- a/app/assets/javascripts/notes/stores/modules/index.js +++ b/app/assets/javascripts/notes/stores/modules/index.js @@ -5,6 +5,7 @@ import mutations from '../mutations'; export default () => ({ state: { discussions: [], + convertedDisscussionIds: [], targetNoteHash: null, lastFetchedAt: null, diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index d167f8ef421..23ef843bb75 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -266,7 +266,7 @@ export default { }, [types.CONVERT_TO_DISCUSSION](state, discussionId) { - const discussion = utils.findNoteObjectById(state.discussions, discussionId); - Object.assign(discussion, { individual_note: false }); + const convertedDisscussionIds = [...state.convertedDisscussionIds, discussionId]; + Object.assign(state, { convertedDisscussionIds }); }, }; diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/commits_header.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/commits_header.vue index a1d3a09cca4..33963d5e1e6 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/commits_header.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/commits_header.vue @@ -73,14 +73,14 @@ export default { <gl-button :aria-label="ariaLabel" variant="blank" - class="commit-edit-toggle mr-2" + class="commit-edit-toggle square s24 mr-2" @click.stop="toggle()" > <icon :name="collapseIcon" :size="16" /> </gl-button> <span v-if="expanded">{{ __('Collapse') }}</span> <span v-else> - <span v-html="message"></span> + <span class="vertical-align-middle" v-html="message"></span> <gl-button variant="link" class="modify-message-button"> {{ modifyLinkMessage }} </gl-button> diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index c78876d9e2a..790d438c7e2 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -1017,3 +1017,8 @@ z-index: 99999; background: $black-transparent; } + +.source-branch-removal-status { + padding-left: 50px; + padding-bottom: $gl-padding; +} diff --git a/app/controllers/concerns/send_file_upload.rb b/app/controllers/concerns/send_file_upload.rb index 9ca54c5519b..28e4cece548 100644 --- a/app/controllers/concerns/send_file_upload.rb +++ b/app/controllers/concerns/send_file_upload.rb @@ -3,7 +3,7 @@ module SendFileUpload def send_upload(file_upload, send_params: {}, redirect_params: {}, attachment: nil, proxy: false, disposition: 'attachment') if attachment - response_disposition = ::Gitlab::ContentDisposition.format(disposition: 'attachment', filename: attachment) + response_disposition = ::Gitlab::ContentDisposition.format(disposition: disposition, filename: attachment) # Response-Content-Type will not override an existing Content-Type in # Google Cloud Storage, so the metadata needs to be cleared on GCS for diff --git a/app/models/discussion.rb b/app/models/discussion.rb index f2678e0597d..32529ebf71d 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -17,8 +17,6 @@ class Discussion :for_commit?, :for_merge_request?, - :save, - to: :first_note def project_id diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb index aab0ff93468..b4a661ae5b4 100644 --- a/app/models/individual_note_discussion.rb +++ b/app/models/individual_note_discussion.rb @@ -17,8 +17,12 @@ class IndividualNoteDiscussion < Discussion noteable.supports_replying_to_individual_notes? && Feature.enabled?(:reply_to_individual_notes) end - def convert_to_discussion! - first_note.becomes!(Discussion.note_class).to_discussion + def convert_to_discussion!(save: false) + first_note.becomes!(Discussion.note_class).to_discussion.tap do + # Save needs to be called on first_note instead of the transformed note + # because of https://gitlab.com/gitlab-org/gitlab-ce/issues/57324 + first_note.save if save + end end def reply_attributes diff --git a/app/models/user.rb b/app/models/user.rb index 24101eda0b1..0d52006db28 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -275,6 +275,7 @@ class User < ApplicationRecord scope :confirmed, -> { where.not(confirmed_at: nil) } scope :by_username, -> (usernames) { iwhere(username: Array(usernames).map(&:to_s)) } scope :for_todos, -> (todos) { where(id: todos.select(:user_id)) } + scope :with_emails, -> { preload(:emails) } # Limits the users to those that have TODOs, optionally in the given state. # diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index b975c3a8cb6..5a6e7338b42 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -35,7 +35,7 @@ module Notes if !only_commands && note.save if note.part_of_discussion? && note.discussion.can_convert_to_discussion? - note.discussion.convert_to_discussion!.save(touch: false) + note.discussion.convert_to_discussion!(save: true) end todo_service.new_note(note, current_user) diff --git a/changelogs/unreleased/50559-add-milestone-progress-to-api.yml b/changelogs/unreleased/50559-add-milestone-progress-to-api.yml deleted file mode 100644 index e68e4bd6059..00000000000 --- a/changelogs/unreleased/50559-add-milestone-progress-to-api.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: 'API: Expose milestone progress' -merge_request: 25173 -author: Robert Schilling -type: added diff --git a/changelogs/unreleased/57544-web-ide-new-directory-dialog-shows-file-templates.yml b/changelogs/unreleased/57544-web-ide-new-directory-dialog-shows-file-templates.yml new file mode 100644 index 00000000000..9d9158ca4af --- /dev/null +++ b/changelogs/unreleased/57544-web-ide-new-directory-dialog-shows-file-templates.yml @@ -0,0 +1,5 @@ +--- +title: Do not show file templates when creating a new directory in WebIDE +merge_request: !25119 +author: +type: fixed diff --git a/changelogs/unreleased/57579-gitlab-project-import-fails-sidekiq-undefined-method-import_jid.yml b/changelogs/unreleased/57579-gitlab-project-import-fails-sidekiq-undefined-method-import_jid.yml new file mode 100644 index 00000000000..f7d6a6c4863 --- /dev/null +++ b/changelogs/unreleased/57579-gitlab-project-import-fails-sidekiq-undefined-method-import_jid.yml @@ -0,0 +1,5 @@ +--- +title: Fix import_jid error on project import +merge_request: 25239 +author: +type: fixed diff --git a/doc/api/group_milestones.md b/doc/api/group_milestones.md index 920acaf3e49..7be01ce9c6d 100644 --- a/doc/api/group_milestones.md +++ b/doc/api/group_milestones.md @@ -42,7 +42,6 @@ Example Response: "due_date": "2013-11-29", "start_date": "2013-11-10", "state": "active", - "percentage_complete" : 66, "updated_at": "2013-10-02T09:24:18Z", "created_at": "2013-10-02T09:24:18Z" } diff --git a/doc/api/milestones.md b/doc/api/milestones.md index 21a390442bd..fa8f8a0bcf0 100644 --- a/doc/api/milestones.md +++ b/doc/api/milestones.md @@ -39,7 +39,6 @@ Example Response: "due_date": "2013-11-29", "start_date": "2013-11-10", "state": "active", - "percentage_complete" : 66, "updated_at": "2013-10-02T09:24:18Z", "created_at": "2013-10-02T09:24:18Z" } diff --git a/doc/development/testing_guide/review_apps.md b/doc/development/testing_guide/review_apps.md index 3af97717775..85b47f88cc4 100644 --- a/doc/development/testing_guide/review_apps.md +++ b/doc/development/testing_guide/review_apps.md @@ -143,17 +143,21 @@ thousands of unused Docker images.** **How big are the Kubernetes clusters (`review-apps-ce` and `review-apps-ee`)?** > The clusters are currently set up with a single pool of preemptible nodes, - with a minimum of 1 node and a maximum of 100 nodes. + with a minimum of 1 node and a maximum of 50 nodes. **What are the machine running on the cluster?** - > We're currently using `n1-standard-4` (4 vCPUs, 15 GB memory) machines. + > We're currently using `n1-standard-16` (16 vCPUs, 60 GB memory) machines. **How do we secure this from abuse? Apps are open to the world so we need to find a way to limit it to only us.** > This isn't enabled for forks. +## Other resources + +* [Review Apps integration for CE/EE (presentation)](https://docs.google.com/presentation/d/1QPLr6FO4LduROU8pQIPkX1yfGvD13GEJIBOenqoKxR8/edit?usp=sharing) + [charts-1068]: https://gitlab.com/charts/gitlab/issues/1068 [gitlab-pipeline]: https://gitlab.com/gitlab-org/gitlab-ce/pipelines/44362587 [gitlab:assets:compile]: https://gitlab.com/gitlab-org/gitlab-ce/-/jobs/149511610 diff --git a/doc/integration/github.md b/doc/integration/github.md index eca9aa16499..9bb3579dd84 100644 --- a/doc/integration/github.md +++ b/doc/integration/github.md @@ -21,10 +21,10 @@ To get the credentials (a pair of Client ID and Client Secret), you must registe - Application name: This can be anything. Consider something like `<Organization>'s GitLab` or `<Your Name>'s GitLab` or something else descriptive. - Homepage URL: the URL to your GitLab installation. e.g., `https://gitlab.company.com` - Application description: Fill this in if you wish. - - Authorization callback URL: `http(s)://${YOUR_DOMAIN}/users/auth`. Please make sure the port is included if your GitLab instance is not configured on default port. + - Authorization callback URL: `http(s)://${YOUR_DOMAIN}/users/auth/github/callback`. Please make sure the port is included if your GitLab instance is not configured on default port.  - NOTE: Be sure to append `/users/auth` to the end of the callback URL + NOTE: Be sure to append `/users/auth/github/callback` to the end of the callback URL to prevent a [OAuth2 convert redirect](http://tetraph.com/covert_redirect/) vulnerability. diff --git a/doc/workflow/gitlab_flow.md b/doc/workflow/gitlab_flow.md index a7313082fac..1b9fb504b15 100644 --- a/doc/workflow/gitlab_flow.md +++ b/doc/workflow/gitlab_flow.md @@ -2,314 +2,324 @@ # Introduction to GitLab Flow -Version management with git makes branching and merging much easier than older versioning systems such as SVN. -This allows a wide variety of branching strategies and workflows. -Almost all of these are an improvement over the methods used before git. -But many organizations end up with a workflow that is not clearly defined, overly complex or not integrated with issue tracking systems. -Therefore we propose the GitLab flow as clearly defined set of best practices. -It combines [feature driven development](https://en.wikipedia.org/wiki/Feature-driven_development) and [feature branches](http://martinfowler.com/bliki/FeatureBranch.html) with issue tracking. +Git allows a wide variety of branching strategies and workflows. +Because of this, many organizations end up with workflows that are too complicated, not clearly defined, or not integrated with issue tracking systems. +Therefore, we propose GitLab flow as a clearly defined set of best practices. +It combines [feature-driven development](https://en.wikipedia.org/wiki/Feature-driven_development) and [feature branches](https://martinfowler.com/bliki/FeatureBranch.html) with issue tracking. -Organizations coming to git from other version control systems frequently find it hard to develop an effective workflow. -This article describes the GitLab flow that integrates the git workflow with an issue tracking system. -It offers a simple, transparent and effective way to work with git. +Organizations coming to Git from other version control systems frequently find it hard to develop a productive workflow. +This article describes GitLab flow, which integrates the Git workflow with an issue tracking system. +It offers a simple, transparent, and effective way to work with Git.  -When converting to git you have to get used to the fact that there are three steps before a commit is shared with colleagues. -Most version control systems have only one step, committing from the working copy to a shared server. -In git you add files from the working copy to the staging area. After that you commit them to the local repo. +When converting to Git, you have to get used to the fact that it takes three steps to share a commit with colleagues. +Most version control systems have only one step: committing from the working copy to a shared server. +In Git, you add files from the working copy to the staging area. After that, you commit them to your local repo. The third step is pushing to a shared remote repository. -After getting used to these three steps the branching model becomes the challenge. +After getting used to these three steps, the next challenge is the branching model. - + -Since many organizations new to git have no conventions how to work with it, it can quickly become a mess. -The biggest problem they run into is that many long running branches that each contain part of the changes are around. -People have a hard time figuring out which branch they should develop on or deploy to production. -Frequently the reaction to this problem is to adopt a standardized pattern such as [git flow](http://nvie.com/posts/a-successful-git-branching-model/) and [GitHub flow](http://scottchacon.com/2011/08/31/github-flow.html). -We think there is still room for improvement and will detail a set of practices we call GitLab flow. +Since many organizations new to Git have no conventions for how to work with it, their repositories can quickly become messy. +The biggest problem is that many long-running branches emerge that all contain part of the changes. +People have a hard time figuring out which branch has the latest code, or which branch to deploy to production. +Frequently, the reaction to this problem is to adopt a standardized pattern such as [Git flow](https://nvie.com/posts/a-successful-git-branching-model/) and [GitHub flow](http://scottchacon.com/2011/08/31/github-flow.html). +We think there is still room for improvement. In this document, we describe a set of practices we call GitLab flow. ## Git flow and its problems  -Git flow was one of the first proposals to use git branches and it has gotten a lot of attention. -It advocates a master branch and a separate develop branch as well as supporting branches for features, releases and hotfixes. -The development happens on the develop branch, moves to a release branch and is finally merged into the master branch. -Git flow is a well defined standard but its complexity introduces two problems. -The first problem is that developers must use the develop branch and not master, master is reserved for code that is released to production. -It is a convention to call your default branch master and to mostly branch from and merge to this. -Since most tools automatically make the master branch the default one and display that one by default it is annoying to have to switch to another one. -The second problem of git flow is the complexity introduced by the hotfix and release branches. +Git flow was one of the first proposals to use Git branches, and it has received a lot of attention. +It suggests a `master` branch and a separate `develop` branch, as well as supporting branches for features, releases, and hotfixes. +The development happens on the `develop` branch, moves to a release branch, and is finally merged into the `master` branch. + +Git flow is a well-defined standard, but its complexity introduces two problems. +The first problem is that developers must use the `develop` branch and not `master`. `master` is reserved for code that is released to production. +It is a convention to call your default branch `master` and to mostly branch from and merge to this. +Since most tools automatically use the `master` branch as the default, it is annoying to have to switch to another branch. + +The second problem of Git flow is the complexity introduced by the hotfix and release branches. These branches can be a good idea for some organizations but are overkill for the vast majority of them. -Nowadays most organizations practice continuous delivery which means that your default branch can be deployed. -This means that hotfix and release branches can be prevented including all the ceremony they introduce. +Nowadays, most organizations practice continuous delivery, which means that your default branch can be deployed. +Continuous delivery removes the need for hotfix and release branches, including all the ceremony they introduce. An example of this ceremony is the merging back of release branches. Though specialized tools do exist to solve this, they require documentation and add complexity. -Frequently developers make a mistake and for example changes are only merged into master and not into the develop branch. -The root cause of these errors is that git flow is too complex for most of the use cases. -And doing releases doesn't automatically mean also doing hotfixes. +Frequently, developers make mistakes such as merging changes only into `master` and not into the `develop` branch. +The reason for these errors is that Git flow is too complicated for most use cases. +For example, many projects do releases but don't need to do hotfixes. ## GitHub flow as a simpler alternative  -In reaction to git flow a simpler alternative was detailed, [GitHub flow](https://guides.github.com/introduction/flow/index.html). -This flow has only feature branches and a master branch. -This is very simple and clean, many organizations have adopted it with great success. -Atlassian recommends [a similar strategy](http://blogs.atlassian.com/2014/01/simple-git-workflow-simple/) although they rebase feature branches. -Merging everything into the master branch and deploying often means you minimize the amount of code in 'inventory' which is in line with the lean and continuous delivery best practices. -But this flow still leaves a lot of questions unanswered regarding deployments, environments, releases and integrations with issues. -With GitLab flow we offer additional guidance for these questions. +In reaction to Git flow, GitHub created a simpler alternative. +[GitHub flow](https://guides.github.com/introduction/flow/index.html) has only feature branches and a `master` branch. +This flow is clean and straightforward, and many organizations have adopted it with great success. +Atlassian recommends [a similar strategy](https://www.atlassian.com/blog/archives/simple-git-workflow-simple), although they rebase feature branches. +Merging everything into the `master` branch and frequently deploying means you minimize the amount of unreleased code, which is in line with lean and continuous delivery best practices. +However, this flow still leaves a lot of questions unanswered regarding deployments, environments, releases, and integrations with issues. +With GitLab flow, we offer additional guidance for these questions. ## Production branch with GitLab flow - + -GitHub flow does assume you are able to deploy to production every time you merge a feature branch. -This is possible for e.g. SaaS applications, but there are many cases where this is not possible. -One would be a situation where you are not in control of the exact release moment, for example an iOS application that needs to pass App Store validation. -Another example is when you have deployment windows (workdays from 10am to 4pm when the operations team is at full capacity) but you also merge code at other times. -In these cases you can make a production branch that reflects the deployed code. -You can deploy a new version by merging in master to the production branch. -If you need to know what code is in production you can just checkout the production branch to see. +GitHub flow assumes you can deploy to production every time you merge a feature branch. +While this is possible in some cases, such as SaaS applications, there are many cases where this is not possible. +One case is where you don't control the timing of a release, for example, an iOS application that is released when it passes App Store validation. +Another case is when you have deployment windows — for example, workdays from 10 AM to 4 PM when the operations team is at full capacity — but you also merge code at other times. +In these cases, you can make a production branch that reflects the deployed code. +You can deploy a new version by merging `master` into the production branch. +If you need to know what code is in production, you can just checkout the production branch to see. The approximate time of deployment is easily visible as the merge commit in the version control system. This time is pretty accurate if you automatically deploy your production branch. -If you need a more exact time you can have your deployment script create a tag on each deployment. -This flow prevents the overhead of releasing, tagging and merging that is common to git flow. +If you need a more exact time, you can have your deployment script create a tag on each deployment. +This flow prevents the overhead of releasing, tagging, and merging that happens with Git flow. ## Environment branches with GitLab flow  -It might be a good idea to have an environment that is automatically updated to the master branch. -Only in this case, the name of this environment might differ from the branch name. -Suppose you have a staging environment, a pre-production environment and a production environment. -In this case the master branch is deployed on staging. When someone wants to deploy to pre-production they create a merge request from the master branch to the pre-production branch. -And going live with code happens by merging the pre-production branch into the production branch. -This workflow where commits only flow downstream ensures that everything has been tested on all environments. -If you need to cherry-pick a commit with a hotfix it is common to develop it on a feature branch and merge it into master with a merge request, do not delete the feature branch. -If master is good to go (it should be if you are practicing [continuous delivery](http://martinfowler.com/bliki/ContinuousDelivery.html)) you then merge it to the other branches. -If this is not possible because more manual testing is required you can send merge requests from the feature branch to the downstream branches. +It might be a good idea to have an environment that is automatically updated to the `master` branch. +Only, in this case, the name of this environment might differ from the branch name. +Suppose you have a staging environment, a pre-production environment, and a production environment. +In this case, deploy the `master` branch to staging. +To deploy to pre-production, create a merge request from the `master` branch to the pre-production branch. +Go live by merging the pre-production branch into the production branch. +This workflow, where commits only flow downstream, ensures that everything is tested in all environments. +If you need to cherry-pick a commit with a hotfix, it is common to develop it on a feature branch and merge it into `master` with a merge request. +In this case, do not delete the feature branch yet. +If `master` passes automatic testing, you then merge the feature branch into the other branches. +If this is not possible because more manual testing is required, you can send merge requests from the feature branch to the downstream branches. ## Release branches with GitLab flow  -Only in case you need to release software to the outside world you need to work with release branches. -In this case, each branch contains a minor version (2-3-stable, 2-4-stable, etc.). -The stable branch uses master as a starting point and is created as late as possible. -By branching as late as possible you minimize the time you have to apply bug fixes to multiple branches. -After a release branch is announced, only serious bug fixes are included in the release branch. -If possible these bug fixes are first merged into master and then cherry-picked into the release branch. -This way you can't forget to cherry-pick them into master and encounter the same bug on subsequent releases. -This is called an 'upstream first' policy that is also practiced by [Google](https://www.chromium.org/chromium-os/chromiumos-design-docs/upstream-first) and [Red Hat](https://www.redhat.com/about/news/archive/2013/5/a-community-for-using-openstack-with-red-hat-rdo). -Every time a bug-fix is included in a release branch the patch version is raised (to comply with [Semantic Versioning](http://semver.org/)) by setting a new tag. +You only need to work with release branches if you need to release software to the outside world. +In this case, each branch contains a minor version, for example, 2-3-stable, 2-4-stable, etc. +Create stable branches using `master` as a starting point, and branch as late as possible. +By doing this, you minimize the length of time during which you have to apply bug fixes to multiple branches. +After announcing a release branch, only add serious bug fixes to the branch. +If possible, first merge these bug fixes into `master`, and then cherry-pick them into the release branch. +If you start by merging into the release branch, you might forget to cherry-pick them into `master`, and then you'd encounter the same bug in subsequent releases. +Merging into `master` and then cherry-picking into release is called an "upstream first" policy, which is also practiced by [Google](https://www.chromium.org/chromium-os/chromiumos-design-docs/upstream-first) and [Red Hat](https://www.redhat.com/en/blog/a-community-for-using-openstack-with-red-hat-rdo). +Every time you include a bug fix in a release branch, increase the patch version (to comply with [Semantic Versioning](https://semver.org/)) by setting a new tag. Some projects also have a stable branch that points to the same commit as the latest released branch. -In this flow it is not common to have a production branch (or git flow master branch). +In this flow, it is not common to have a production branch (or Git flow `master` branch). ## Merge/pull requests with GitLab flow - + -Merge or pull requests are created in a git management application and ask an assigned person to merge two branches. -Tools such as GitHub and Bitbucket choose the name pull request since the first manual action would be to pull the feature branch. -Tools such as GitLab and others choose the name merge request since that is the final action that is requested of the assignee. -In this article we'll refer to them as merge requests. +Merge or pull requests are created in a Git management application. They ask an assigned person to merge two branches. +Tools such as GitHub and Bitbucket choose the name "pull request" since the first manual action is to pull the feature branch. +Tools such as GitLab and others choose the name "merge request" since the final action is to merge the feature branch. +In this article, we'll refer to them as merge requests. -If you work on a feature branch for more than a few hours it is good to share the intermediate result with the rest of the team. -This can be done by creating a merge request without assigning it to anyone, instead you mention people in the description or a comment (/cc @mark @susan). -This means it is not ready to be merged but feedback is welcome. +If you work on a feature branch for more than a few hours, it is good to share the intermediate result with the rest of the team. +To do this, create a merge request without assigning it to anyone. +Instead, mention people in the description or a comment, for example, "/cc @mark @susan." +This indicates that the merge request is not ready to be merged yet, but feedback is welcome. Your team members can comment on the merge request in general or on specific lines with line comments. -The merge requests serves as a code review tool and no separate tools such as Gerrit and reviewboard should be needed. -If the review reveals shortcomings anyone can commit and push a fix. -Commonly the person to do this is the creator of the merge/pull request. -The diff in the merge/pull requests automatically updates when new commits are pushed on the branch. +The merge request serves as a code review tool, and no separate code review tools should be needed. +If the review reveals shortcomings, anyone can commit and push a fix. +Usually, the person to do this is the creator of the merge request. +The diff in the merge request automatically updates when new commits are pushed to the branch. + +When you are ready for your feature branch to be merged, assign the merge request to the person who knows most about the codebase you are changing. +Also, mention any other people from whom you would like feedback. +After the assigned person feels comfortable with the result, they can merge the branch. +If the assigned person does not feel comfortable, they can request more changes or close the merge request without merging. + +In GitLab, it is common to protect the long-lived branches, e.g., the `master` branch, so that [most developers can't modify them](../permissions/permissions.md). +So, if you want to merge into a protected branch, assign your merge request to someone with maintainer permissions. -When you feel comfortable with it to be merged you assign it to the person that knows most about the codebase you are changing and mention any other people you would like feedback from. -There is room for more feedback and after the assigned person feels comfortable with the result the branch is merged. -If the assigned person does not feel comfortable they can close the merge request without merging. +After you merge a feature branch, you should remove it from the source control software. +In GitLab, you can do this when merging. +Removing finished branches ensures that the list of branches shows only work in progress. +It also ensures that if someone reopens the issue, they can use the same branch name without causing problems. -In GitLab it is common to protect the long-lived branches (e.g. the master branch) so that normal developers [can't modify these protected branches](http://docs.gitlab.com/ce/permissions/permissions.html). -So if you want to merge it into a protected branch you assign it to someone with maintainer authorizations. +NOTE: **Note:** +When you reopen an issue you need to create a new merge request. + + ## Issue tracking with GitLab flow - + GitLab flow is a way to make the relation between the code and the issue tracker more transparent. -Any significant change to the code should start with an issue where the goal is described. -Having a reason for every code change is important to inform everyone on the team and to help people keep the scope of a feature branch small. -In GitLab each change to the codebase starts with an issue in the issue tracking system. -If there is no issue yet it should be created first provided there is significant work involved (more than 1 hour). -For many organizations this will be natural since the issue will have to be estimated for the sprint. -Issue titles should describe the desired state of the system, e.g. "As an administrator I want to remove users without receiving an error" instead of "Admin can't remove users.". - -When you are ready to code you start a branch for the issue from the master branch. -The name of this branch should start with the issue number, for example '15-require-a-password-to-change-it'. - -When you are done or want to discuss the code you open a merge request. -This is an online place to discuss the change and review the code. -Opening a merge request is a manual action since you do not always want to merge a new branch you push, it could be a long-running environment or release branch. -If you open the merge request but do not assign it to anyone it is a 'Work In Progress' merge request. -These are used to discuss the proposed implementation but are not ready for inclusion in the master branch yet. -_Pro tip:_ Start the title of the merge request with `[WIP]` or `WIP:` to prevent it from being merged before it's ready. - -When the author thinks the code is ready the merge request is assigned to reviewer. -The reviewer presses the merge button when they think the code is ready for inclusion in the master branch. -In this case the code is merged and a merge commit is generated that makes this event easily visible later on. -Merge requests always create a merge commit even when the commit could be added without one. -This merge strategy is called 'no fast-forward' in git. -After the merge the feature branch is deleted since it is no longer needed, in GitLab this deletion is an option when merging. +Any significant change to the code should start with an issue that describes the goal. +Having a reason for every code change helps to inform the rest of the team and to keep the scope of a feature branch small. +In GitLab, each change to the codebase starts with an issue in the issue tracking system. +If there is no issue yet, create the issue, as long as the change will take a significant amount of work, i.e., more than 1 hour. +In many organizations, raising an issue is part of the development process because they are used in sprint planning. +The issue title should describe the desired state of the system. +For example, the issue title "As an administrator, I want to remove users without receiving an error" is better than "Admin can't remove users." + +When you are ready to code, create a branch for the issue from the `master` branch. +This branch is the place for any work related to this change. + +NOTE: **Note:** +The name of a branch might be dictated by organizational standards. +For example, in GitLab, any branches in GitLab EE that are equivalent to branches in GitLab CE [must end in `-ee`](https://docs.gitlab.com/ee/development/automatic_ce_ee_merge.html#cherry-picking-from-ce-to-ee). + +When you are done or want to discuss the code, open a merge request. +A merge request is an online place to discuss the change and review the code. + +If you open the merge request but do not assign it to anyone, it is a "Work In Progress" merge request. +These are used to discuss the proposed implementation but are not ready for inclusion in the `master` branch yet. +Start the title of the merge request with "[WIP]" or "WIP:" to prevent it from being merged before it's ready. + +When you think the code is ready, assign the merge request to a reviewer. +The reviewer can merge the changes when they think the code is ready for inclusion in the `master` branch. +When they press the merge button, GitLab merges the code and creates a merge commit that makes this event easily visible later on. +Merge requests always create a merge commit, even when the branch could be merged without one. +This merge strategy is called "no fast-forward" in Git. +After the merge, delete the feature branch since it is no longer needed. +In GitLab, this deletion is an option when merging. Suppose that a branch is merged but a problem occurs and the issue is reopened. -In this case it is no problem to reuse the same branch name since it was deleted when the branch was merged. -At any time there is at most one branch for every issue. +In this case, it is no problem to reuse the same branch name since the first branch was deleted when it was merged. +At any time, there is at most one branch for every issue. It is possible that one feature branch solves more than one issue. ## Linking and closing issues from merge requests  -Linking to issues can happen by mentioning them in commit messages (fixes #14, closes #67, etc.) or in the merge request description. -GitLab then creates links to the mentioned issues and creates comments in the corresponding issues linking back to the merge request. +Link to issues by mentioning them in commit messages or the description of a merge request, for example, "Fixes #16" or "Duck typing is preferred. See #12." +GitLab then creates links to the mentioned issues and creates comments in the issues linking back to the merge request. -These issues are closed once code is merged into the default branch. +To automatically close linked issues, mention them with the words "fixes" or "closes," for example, "fixes #14" or "closes #67." GitLab closes these issues when the code is merged into the default branch. -If you only want to make the reference without closing the issue you can also just mention it: "Duck typing is preferred. #12". - -If you have an issue that spans across multiple repositories, the best thing is to create an issue for each repository and link all issues to a parent issue. +If you have an issue that spans across multiple repositories, create an issue for each repository and link all issues to a parent issue. ## Squashing commits with rebase  -With git you can use an interactive rebase (`rebase -i`) to squash multiple commits into one and reorder them. -In GitLab EE and .com you can also [rebase before merge](http://docs.gitlab.com/ee/workflow/rebase_before_merge.html) from the web interface. -This functionality is useful if you made a couple of commits for small changes during development and want to replace them with a single commit or if you want to make the order more logical. -However you should never rebase commits you have pushed to a remote server. -Somebody can have referred to the commits or cherry-picked them. -When you rebase you change the identifier (SHA-1) of the commit and this is confusing. -If you do that the same change will be known under multiple identifiers and this can cause much confusion. -If people already reviewed your code it will be hard for them to review only the improvements you made since then if you have rebased everything into one commit. -Another reasons not to rebase is that you lose authorship information, maybe someone created a merge request, another person pushed a commit on there to improve it and a third one merged it. -In this case rebasing all the commits into one prevent the other authors from being properly attributed and sharing part of the [git blame](https://git-scm.com/docs/git-blame). - -People are encouraged to commit often and to frequently push to the remote repository so other people are aware what everyone is working on. -This will lead to many commits per change which makes the history harder to understand. -But the advantages of having stable identifiers outweigh this drawback. -And to understand a change in context one can always look at the merge commit that groups all the commits together when the code is merged into the master branch. - -After you merge multiple commits from a feature branch into the master branch this is harder to undo. -If you had squashed all the commits into one you could have just reverted this commit but as we indicated you should not rebase commits after they are pushed. -Fortunately [reverting a merge made some time ago](https://git-scm.com/blog/2010/03/02/undoing-merges.html) can be done with git. -This however, requires having specific merge commits for the commits your want to revert. -If you revert a merge and you change your mind, revert the revert instead of merging again since git will not allow you to merge the code again otherwise. - -Being able to revert a merge is a good reason always to create a merge commit when you merge manually with the `--no-ff` option. -Git management software will always create a merge commit when you accept a merge request. - -## Do not order commits with rebase +With Git, you can use an interactive rebase (`rebase -i`) to squash multiple commits into one or reorder them. +This functionality is useful if you want to replace a couple of small commits with a single commit, or if you want to make the order more logical. + +However, you should never rebase commits you have pushed to a remote server. +Rebasing creates new commits for all your changes, which can cause confusion because the same change would have multiple identifiers. +It also causes merge errors for anyone working on the same branch because their history would not match with yours. +Also, if someone has already reviewed your code, rebasing makes it hard to tell what changed since the last review. + +You should also never rebase commits authored by other people. +Not only does this rewrite history, but it also loses authorship information. +Rebasing prevents the other authors from being attributed and sharing part of the [`git blame`](https://git-scm.com/docs/git-blame). + +If a merge involves many commits, it may seem more difficult to undo. +You might think to solve this by squashing all the changes into one commit before merging, but as discussed earlier, it is a bad idea to rebase commits that you have already pushed. +Fortunately, there is an easy way to undo a merge with all its commits. +The way to do this is by reverting the merge commit. +Preserving this ability to revert a merge is a good reason to always use the "no fast-forward" (`--no-ff`) strategy when you merge manually. + +NOTE: **Note:** +If you revert a merge commit and then change your mind, revert the revert commit to redo the merge. +Git does not allow you to merge the code again otherwise. + +## Reducing merge commits in feature branches  -With git you can also rebase your feature branch commits to order them after the commits on the master branch. -This prevents creating a merge commit when merging master into your feature branch and creates a nice linear history. -However, just like with squashing you should never rebase commits you have pushed to a remote server. -This makes it impossible to rebase work in progress that you already shared with your team which is something we recommend. -When using rebase to keep your feature branch updated you [need to resolve similar conflicts again and again](https://blogs.atlassian.com/2013/10/git-team-workflows-merge-or-rebase/). -You can reuse recorded resolutions (rerere) sometimes, but without rebasing you only have to solve the conflicts one time and you’re set. -There has to be a better way to avoid many merge commits. - -The way to prevent creating many merge commits is to not frequently merge master into the feature branch. -We'll discuss the three reasons to merge in master: leveraging code, merge conflicts, and long running branches. -If you need to leverage some code that was introduced in master after you created the feature branch you can sometimes solve this by just cherry-picking a commit. -If your feature branch has a merge conflict, creating a merge commit is a normal way of solving this. -You can prevent some merge conflicts by using [gitattributes](http://git-scm.com/docs/gitattributes) for files that can be in a random order. -For example in GitLab our changelog file is specified in .gitattributes as `CHANGELOG.md merge=union` so that there are fewer merge conflicts in it. -The last reason for creating merge commits is having long lived branches that you want to keep up to date with the latest state of the project. -Martin Fowler, in [his article about feature branches](http://martinfowler.com/bliki/FeatureBranch.html) talks about this Continuous Integration (CI). -At GitLab we are guilty of confusing CI with branch testing. Quoting Martin Fowler: "I've heard people say they are doing CI because they are running builds, perhaps using a CI server, on every branch with every commit. -That's continuous building, and a Good Thing, but there's no integration, so it's not CI.". -The solution to prevent many merge commits is to keep your feature branches short-lived, the vast majority should take less than one day of work. -If your feature branches commonly take more than a day of work, look into ways to create smaller units of work and/or use [feature toggles](http://martinfowler.com/bliki/FeatureToggle.html). -As for the long running branches that take more than one day there are two strategies. -In a CI strategy you can merge in master at the start of the day to prevent painful merges at a later time. -In a synchronization point strategy you only merge in from well defined points in time, for example a tagged release. -This strategy is [advocated by Linus Torvalds](https://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html) because the state of the code at these points is better known. - -In conclusion, we can say that you should try to prevent merge commits, but not eliminate them. -Your codebase should be clean but your history should represent what actually happened. -Developing software happen in small messy steps and it is OK to have your history reflect this. -You can use tools to view the network graphs of commits and understand the messy history that created your code. -If you rebase code the history is incorrect, and there is no way for tools to remedy this because they can't deal with changing commit identifiers. +Having lots of merge commits can make your repository history messy. +Therefore, you should try to avoid merge commits in feature branches. +Often, people avoid merge commits by just using rebase to reorder their commits after the commits on the `master` branch. +Using rebase prevents a merge commit when merging `master` into your feature branch, and it creates a neat linear history. +However, as discussed in [the section about rebasing](#squashing-commits-with-rebase), you should never rebase commits you have pushed to a remote server. +This restriction makes it impossible to rebase work in progress that you already shared with your team, which is something we recommend. -## Award emojis on issues and merge requests +Rebasing also creates more work, since every time you rebase, you have to resolve similar conflicts. +Sometimes you can reuse recorded resolutions (`rerere`), but merging is better since you only have to resolve conflicts once. +Atlassian has a more thorough explanation of the tradeoffs between merging and rebasing [on their blog](https://www.atlassian.com/blog/git/git-team-workflows-merge-or-rebase). - +A good way to prevent creating many merge commits is to not frequently merge `master` into the feature branch. +There are three reasons to merge in `master`: utilizing new code, resolving merge conflicts, and updating long-running branches. -It is common to voice approval or disapproval by using +1 or -1. In GitLab you -can use emojis to give a virtual high five on issues and merge requests. +If you need to utilize some code that was introduced in `master` after you created the feature branch, you can often solve this by just cherry-picking a commit. -## Pushing and removing branches +If your feature branch has a merge conflict, creating a merge commit is a standard way of solving this. - +NOTE: **Note:** +Sometimes you can use .gitattributes to reduce merge conflicts. +For example, you can set your changelog file to use the [union merge driver](https://git-scm.com/docs/gitattributes#gitattributes-union) so that multiple new entries don't conflict with each other. -We recommend that people push their feature branches frequently, even when they are not ready for review yet. -By doing this you prevent team members from accidentally starting to work on the same issue. -Of course this situation should already be prevented by assigning someone to the issue in the issue tracking software. -However sometimes one of the two parties forgets to assign someone in the issue tracking software. -After a branch is merged it should be removed from the source control software. -In GitLab and similar systems this is an option when merging. -This ensures that the branch overview in the repository management software shows only work in progress. -This also ensures that when someone reopens the issue a new branch with the same name can be used without problem. -When you reopen an issue you need to create a new merge request. +The last reason for creating merge commits is to keep long-running feature branches up-to-date with the latest state of the project. +The solution here is to keep your feature branches short-lived. +Most feature branches should take less than one day of work. +If your feature branches often take more than a day of work, try to split your features into smaller units of work. + +If you need to keep a feature branch open for more than a day, there are a few strategies to keep it up-to-date. +One option is to use continuous integration (CI) to merge in `master` at the start of the day. +Another option is to only merge in from well-defined points in time, for example, a tagged release. +You could also use [feature toggles](https://martinfowler.com/bliki/FeatureToggle.html) to hide incomplete features so you can still merge back into `master` every day. + +> **Note:** Don't confuse automatic branch testing with continuous integration. +> Martin Fowler makes this distinction in [his article about feature branches](https://martinfowler.com/bliki/FeatureBranch.html): +> +> "I've heard people say they are doing CI because they are running builds, perhaps using a CI server, on every branch with every commit. +> That's continuous building, and a Good Thing, but there's no *integration*, so it's not CI." + +In conclusion, you should try to prevent merge commits, but not eliminate them. +Your codebase should be clean, but your history should represent what actually happened. +Developing software happens in small, messy steps, and it is OK to have your history reflect this. +You can use tools to view the network graphs of commits and understand the messy history that created your code. +If you rebase code, the history is incorrect, and there is no way for tools to remedy this because they can't deal with changing commit identifiers. + +## Commit often and push frequently + +Another way to make your development work easier is to commit often. +Every time you have a working set of tests and code, you should make a commit. +Splitting up work into individual commits provides context for developers looking at your code later. +Smaller commits make it clear how a feature was developed, and they make it easy to roll back to a specific good point in time or to revert one code change without reverting several unrelated changes. + +Committing often also makes it easy to share your work, which is important so that everyone is aware of what you are working on. +You should push your feature branch frequently, even when it is not yet ready for review. +By sharing your work in a feature branch or [a merge request](#mergepull-requests-with-gitlab-flow), you prevent your team members from duplicating work. +Sharing your work before it's complete also allows for discussion and feedback about the changes, which can help improve the code before it gets to review. -## Committing often and with the right message +## How to write a good commit message  -We recommend to commit early and often. -Each time you have a functioning set of tests and code a commit can be made. -The advantage is that when an extension or refactor goes wrong it is easy to revert to a working version. -This is quite a change for programmers that used SVN before, they used to commit when their work was ready to share. -The trick is to use the merge/pull request with multiple commits when your work is ready to share. -The commit message should reflect your intention, not the contents of the commit. -The contents of the commit can be easily seen anyway, the question is why you did it. -An example of a good commit message is: "Combine templates to dry up the user views.". -Some words that are bad commit messages because they don't contain much information are: change, improve and refactor. -The word fix or fixes is also a red flag, unless it comes after the commit sentence and references an issue number. -To see more information about the formatting of commit messages please see this great [blog post by Tim Pope](http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html). +A commit message should reflect your intention, not just the contents of the commit. +It is easy to see the changes in a commit, so the commit message should explain why you made those changes. +An example of a good commit message is: "Combine templates to reduce duplicate code in the user views." +The words "change," "improve," "fix," and "refactor" don't add much information to a commit message. +For example, "Improve XML generation" could be better written as "Properly escape special characters in XML generation." +For more information about formatting commit messages, please see this excellent [blog post by Tim Pope](https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html). ## Testing before merging - - -In old workflows the Continuous Integration (CI) server commonly ran tests on the master branch only. -Developers had to ensure their code did not break the master branch. -When using GitLab flow developers create their branches from this master branch so it is essential it is green. -Therefore each merge request must be tested before it is accepted. -CI software like Travis and GitLab CI show the build results right in the merge request itself to make this easy. -One drawback is that they are testing the feature branch itself and not the merged result. -What one can do to improve this is to test the merged result itself. -The problem is that the merge result changes every time something is merged into master. -Retesting on every commit to master is computationally expensive and means you are more frequently waiting for test results. -If there are no merge conflicts and the feature branches are short lived the risk is acceptable. -If there are merge conflicts you merge the master branch into the feature branch and the CI server will rerun the tests. -If you have long lived feature branches that last for more than a few days you should make your issues smaller. + -## Working with feature branches +In old workflows, the continuous integration (CI) server commonly ran tests on the `master` branch only. +Developers had to ensure their code did not break the `master` branch. +When using GitLab flow, developers create their branches from this `master` branch, so it is essential that it never breaks. +Therefore, each merge request must be tested before it is accepted. +CI software like Travis CI and GitLab CI show the build results right in the merge request itself to make this easy. - +There is one drawback to testing merge requests: the CI server only tests the feature branch itself, not the merged result. +Ideally, the server could also test the `master` branch after each change. +However, retesting on every commit to `master` is computationally expensive and means you are more frequently waiting for test results. +Since feature branches should be short-lived, testing just the branch is an acceptable risk. +If new commits in `master` cause merge conflicts with the feature branch, merge `master` back into the branch to make the CI server re-run the tests. +As said before, if you often have feature branches that last for more than a few days, you should make your issues smaller. -When initiating a feature branch, always start with an up to date master to branch off from. -If you know beforehand that your work absolutely depends on another branch you can also branch from there. -If you need to merge in another branch after starting explain the reason in the merge commit. -If you have not pushed your commits to a shared location yet you can also rebase on master or another feature branch. -Do not merge in upstream if your code will work and merge cleanly without doing so, Linus even says that [you should never merge in upstream at random points, only at major releases](https://lwn.net/Articles/328438/). -Merging only when needed prevents creating merge commits in your feature branch that later end up littering the master history. +## Working with feature branches -### References + -- [Git Flow by Vincent Driessen](http://nvie.com/posts/a-successful-git-branching-model/) +When creating a feature branch, always branch from an up-to-date `master`. +If you know before you start that your work depends on another branch, you can also branch from there. +If you need to merge in another branch after starting, explain the reason in the merge commit. +If you have not pushed your commits to a shared location yet, you can also incorporate changes by rebasing on `master` or another feature branch. +Do not merge from upstream again if your code can work and merge cleanly without doing so. +Merging only when needed prevents creating merge commits in your feature branch that later end up littering the `master` history. diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 0ef56067b95..173e86dfd3b 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -502,9 +502,6 @@ module API expose :state, :created_at, :updated_at expose :due_date expose :start_date - expose :percentage_complete do |milestone, options| - milestone.percent_complete(options[:current_user]) - end expose :web_url do |milestone, _options| Gitlab::UrlBuilder.build(milestone) diff --git a/lib/api/milestone_responses.rb b/lib/api/milestone_responses.rb index 4c68c568aaa..a0ca39b69d4 100644 --- a/lib/api/milestone_responses.rb +++ b/lib/api/milestone_responses.rb @@ -35,19 +35,19 @@ module API milestones = filter_by_iid(milestones, params[:iids]) if params[:iids].present? milestones = filter_by_search(milestones, params[:search]) if params[:search] - present paginate(milestones), with: Entities::Milestone, current_user: current_user + present paginate(milestones), with: Entities::Milestone end def get_milestone_for(parent) milestone = parent.milestones.find(params[:milestone_id]) - present milestone, with: Entities::Milestone, current_user: current_user + present milestone, with: Entities::Milestone end def create_milestone_for(parent) milestone = ::Milestones::CreateService.new(parent, current_user, declared_params).execute if milestone.valid? - present milestone, with: Entities::Milestone, current_user: current_user + present milestone, with: Entities::Milestone else render_api_error!("Failed to create milestone #{milestone.errors.messages}", 400) end @@ -60,7 +60,7 @@ module API milestone = ::Milestones::UpdateService.new(parent, current_user, milestone_params).execute(milestone) if milestone.valid? - present milestone, with: Entities::Milestone, current_user: current_user + present milestone, with: Entities::Milestone else render_api_error!("Failed to update milestone #{milestone.errors.messages}", 400) end diff --git a/lib/banzai/filter/footnote_filter.rb b/lib/banzai/filter/footnote_filter.rb index 97527976437..de133774dfa 100644 --- a/lib/banzai/filter/footnote_filter.rb +++ b/lib/banzai/filter/footnote_filter.rb @@ -29,21 +29,30 @@ module Banzai # Sanitization stripped off the section wrapper - add it back in first_footnote.parent.wrap('<section class="footnotes">') rand_suffix = "-#{random_number}" + modified_footnotes = {} doc.css('sup > a[id]').each do |link_node| ref_num = link_node[:id].delete_prefix(FOOTNOTE_LINK_ID_PREFIX) footnote_node = doc.at_css("li[id=#{fn_id(ref_num)}]") - backref_node = footnote_node.at_css("a[href=\"##{fnref_id(ref_num)}\"]") - if ref_num =~ INTEGER_PATTERN && footnote_node && backref_node - link_node[:href] += rand_suffix - link_node[:id] += rand_suffix - footnote_node[:id] += rand_suffix - backref_node[:href] += rand_suffix + if INTEGER_PATTERN.match?(ref_num) && (footnote_node || modified_footnotes[ref_num]) + link_node[:href] += rand_suffix + link_node[:id] += rand_suffix # Sanitization stripped off class - add it back in link_node.parent.append_class('footnote-ref') - backref_node.append_class('footnote-backref') + + unless modified_footnotes[ref_num] + footnote_node[:id] += rand_suffix + backref_node = footnote_node.at_css("a[href=\"##{fnref_id(ref_num)}\"]") + + if backref_node + backref_node[:href] += rand_suffix + backref_node.append_class('footnote-backref') + end + + modified_footnotes[ref_num] = true + end end end diff --git a/lib/gitlab/import_export/shared.rb b/lib/gitlab/import_export/shared.rb index 947caaaefee..725c1101d70 100644 --- a/lib/gitlab/import_export/shared.rb +++ b/lib/gitlab/import_export/shared.rb @@ -61,7 +61,7 @@ module Gitlab def log_base_data { importer: 'Import/Export', - import_jid: @project&.import_state&.import_jid, + import_jid: @project&.import_state&.jid, project_id: @project&.id, project_path: @project&.full_path } diff --git a/qa/spec/spec_helper.rb b/qa/spec/spec_helper.rb index 3e7a6dd26ee..f1b4203d422 100644 --- a/qa/spec/spec_helper.rb +++ b/qa/spec/spec_helper.rb @@ -19,26 +19,16 @@ RSpec.configure do |config| end end - config.before do |example| - QA::Runtime::Logger.debug("Starting test: #{example.full_description}") if QA::Runtime::Env.debug? - - # If quarantine is tagged, skip tests that have other metadata unless - # they're also tagged. This lets us run quarantined tests in a particular - # category without running tests in other categories. - # E.g., if a test is tagged 'smoke' and 'quarantine', and another is tagged - # 'ldap' and 'quarantine', if we wanted to run just quarantined smoke tests - # using `--tag quarantine --tag smoke`, without this check we'd end up - # running that ldap test as well. - if config.inclusion_filter[:quarantine] - skip("Running tests tagged with all of #{config.inclusion_filter.rules.keys}") unless quarantine_and_optional_other_tag?(example, config) + config.before(:context) do + if self.class.metadata.keys.include?(:quarantine) + skip_or_run_quarantined_tests(self.class.metadata.keys, config.inclusion_filter.rules.keys) end end - config.before(:each, :quarantine) do |example| - # Skip tests in quarantine unless we explicitly focus on them - # We could use an exclusion filter, but this way the test report will list - # the quarantined tests when they're not run so that we're aware of them - skip('In quarantine') unless config.inclusion_filter[:quarantine] + config.before do |example| + QA::Runtime::Logger.debug("Starting test: #{example.full_description}") if QA::Runtime::Env.debug? + + skip_or_run_quarantined_tests(example.metadata.keys, config.inclusion_filter.rules.keys) end config.expect_with :rspec do |expectations| @@ -57,18 +47,41 @@ RSpec.configure do |config| Kernel.srand config.seed end +# Skip tests in quarantine unless we explicitly focus on them. +# Skip the entire context if a context is tagged. This avoids running before +# blocks unnecessarily. +# If quarantine is focussed, skip tests/contexts that have other metadata +# unless they're also focussed. This lets us run quarantined tests in a +# particular category without running tests in other categories. +# E.g., if a test is tagged 'smoke' and 'quarantine', and another is tagged +# 'ldap' and 'quarantine', if we wanted to run just quarantined smoke tests +# using `--tag quarantine --tag smoke`, without this check we'd end up +# running that ldap test as well. +# We could use an exclusion filter, but this way the test report will list +# the quarantined tests when they're not run so that we're aware of them +def skip_or_run_quarantined_tests(metadata_keys, filter_keys) + included_filters = filters_other_than_quarantine(filter_keys) + + if filter_keys.include?(:quarantine) + skip("Only running tests tagged with :quarantine and any of #{included_filters}") unless quarantine_and_optional_other_tag?(metadata_keys, included_filters) + else + skip('In quarantine') if metadata_keys.include?(:quarantine) + end +end + +def filters_other_than_quarantine(filter_keys) + filter_keys.reject { |key| key == :quarantine } +end + # Checks if a test has the 'quarantine' tag and other tags in the inclusion filter. # # Returns true if -# - the example metadata includes the quarantine tag -# - and the metadata and inclusion filter both have any other tag -# - or no other tags are in the inclusion filter -def quarantine_and_optional_other_tag?(example, config) - return false unless example.metadata.keys.include? :quarantine - - filters_other_than_quarantine = config.inclusion_filter.rules.keys.reject { |key| key == :quarantine } - - return true if filters_other_than_quarantine.empty? +# - the metadata includes the quarantine tag +# - and the metadata and inclusion filter both have any other tag +# - or no other tags are in the inclusion filter +def quarantine_and_optional_other_tag?(metadata_keys, included_filters) + return false unless metadata_keys.include? :quarantine + return true if included_filters.empty? - filters_other_than_quarantine.any? { |key| example.metadata.keys.include? key } + included_filters.any? { |key| metadata_keys.include? key } end diff --git a/qa/spec/spec_helper_spec.rb b/qa/spec/spec_helper_spec.rb index f001200fb52..2427999e110 100644 --- a/qa/spec/spec_helper_spec.rb +++ b/qa/spec/spec_helper_spec.rb @@ -10,79 +10,79 @@ describe 'rspec config tests' do end end + context 'default' do + it_behaves_like 'passing tests' + end + context 'foo', :foo do it_behaves_like 'passing tests' end - context 'default' do + context 'quarantine', :quarantine do + it_behaves_like 'passing tests' + end + + context 'bar quarantine', :bar, :quarantine do it_behaves_like 'passing tests' end end end - context 'default config' do - it 'tests are skipped if in quarantine' do + context 'with no tags focussed' do + before do group.run + end - foo_context = group.children.find { |c| c.description == "foo" } - foo_examples = foo_context.descendant_filtered_examples - expect(foo_examples.count).to eq(2) - - ex = foo_examples.find { |e| e.description == "not in quarantine" } - expect(ex.execution_result.status).to eq(:passed) + context 'in a context tagged :foo' do + it 'skips tests in quarantine' do + context = group.children.find { |c| c.description == "foo" } + examples = context.descendant_filtered_examples + expect(examples.count).to eq(2) - ex = foo_examples.find { |e| e.description == "in quarantine" } - expect(ex.execution_result.status).to eq(:pending) - expect(ex.execution_result.pending_message).to eq('In quarantine') + ex = examples.find { |e| e.description == "not in quarantine" } + expect(ex.execution_result.status).to eq(:passed) - default_context = group.children.find { |c| c.description == "default" } - default_examples = default_context.descendant_filtered_examples - expect(default_examples.count).to eq(2) + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:pending) + expect(ex.execution_result.pending_message).to eq('In quarantine') + end + end - ex = default_examples.find { |e| e.description == "not in quarantine" } - expect(ex.execution_result.status).to eq(:passed) + context 'in an untagged context' do + it 'skips tests in quarantine' do + context = group.children.find { |c| c.description == "default" } + examples = context.descendant_filtered_examples + expect(examples.count).to eq(2) - ex = default_examples.find { |e| e.description == "in quarantine" } - expect(ex.execution_result.status).to eq(:pending) - expect(ex.execution_result.pending_message).to eq('In quarantine') - end - end + ex = examples.find { |e| e.description == "not in quarantine" } + expect(ex.execution_result.status).to eq(:passed) - context "with 'quarantine' tagged" do - before do - RSpec.configure do |config| - config.inclusion_filter = :quarantine - end - end - after do - RSpec.configure do |config| - config.inclusion_filter.clear + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:pending) + expect(ex.execution_result.pending_message).to eq('In quarantine') end end - it "only quarantined tests are run" do - group.run - - foo_context = group.children.find { |c| c.description == "foo" } - foo_examples = foo_context.descendant_filtered_examples - expect(foo_examples.count).to be(1) - - ex = foo_examples.find { |e| e.description == "in quarantine" } - expect(ex.execution_result.status).to eq(:passed) + context 'in a context tagged :quarantine' do + it 'skips all tests' do + context = group.children.find { |c| c.description == "quarantine" } + examples = context.descendant_filtered_examples + expect(examples.count).to eq(2) - default_context = group.children.find { |c| c.description == "default" } - default_examples = default_context.descendant_filtered_examples - expect(default_examples.count).to be(1) + ex = examples.find { |e| e.description == "not in quarantine" } + expect(ex.execution_result.status).to eq(:pending) - ex = default_examples.find { |e| e.description == "in quarantine" } - expect(ex.execution_result.status).to eq(:passed) + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:pending) + expect(ex.execution_result.pending_message).to eq('In quarantine') + end end end - context "with 'foo' tagged" do + context 'with :quarantine focussed' do before do RSpec.configure do |config| - config.inclusion_filter = :foo + config.inclusion_filter = :quarantine end group.run @@ -93,30 +93,50 @@ describe 'rspec config tests' do end end - it "tests are not run if not tagged 'foo'" do - default_context = group.children.find { |c| c.description == "default" } - expect(default_context.descendant_filtered_examples.count).to eq(0) + context 'in an untagged context' do + it 'only runs quarantined tests' do + context = group.children.find { |c| c.description == "default" } + examples = context.descendant_filtered_examples + expect(examples.count).to be(1) + + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:passed) + end end - it "tests are skipped if in quarantine" do - foo_context = group.children.find { |c| c.description == "foo" } - foo_examples = foo_context.descendant_filtered_examples - expect(foo_examples.count).to eq(2) + context 'in a context tagged :foo' do + it 'only runs quarantined tests' do + context = group.children.find { |c| c.description == "foo" } + examples = context.descendant_filtered_examples + expect(examples.count).to be(1) - ex = foo_examples.find { |e| e.description == "not in quarantine" } - expect(ex.execution_result.status).to eq(:passed) + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:passed) + end + end + + context 'in a context tagged :quarantine' do + it 'runs all tests' do + context = group.children.find { |c| c.description == "quarantine" } + examples = context.descendant_filtered_examples + expect(examples.count).to be(2) + + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:passed) - ex = foo_examples.find { |e| e.description == "in quarantine" } - expect(ex.execution_result.status).to eq(:pending) - expect(ex.execution_result.pending_message).to eq('In quarantine') + ex = examples.find { |e| e.description == "not in quarantine" } + expect(ex.execution_result.status).to eq(:passed) + end end end - context "with 'quarantine' and 'foo' tagged" do + context 'with a non-quarantine tag (:foo) focussed' do before do RSpec.configure do |config| - config.inclusion_filter = { quarantine: true, foo: true } + config.inclusion_filter = :foo end + + group.run end after do RSpec.configure do |config| @@ -124,38 +144,43 @@ describe 'rspec config tests' do end end - it 'of tests tagged foo, only tests in quarantine run' do - group.run + context 'in an untagged context' do + it 'runs no tests' do + context = group.children.find { |c| c.description == "default" } + expect(context.descendant_filtered_examples.count).to eq(0) + end + end - foo_context = group.children.find { |c| c.description == "foo" } - foo_examples = foo_context.descendant_filtered_examples - expect(foo_examples.count).to eq(2) + context 'in a context tagged :foo' do + it 'skips quarantined tests' do + context = group.children.find { |c| c.description == "foo" } + examples = context.descendant_filtered_examples + expect(examples.count).to be(2) - ex = foo_examples.find { |e| e.description == "not in quarantine" } - expect(ex.execution_result.status).to eq(:pending) - expect(ex.execution_result.pending_message).to eq('Running tests tagged with all of [:quarantine, :foo]') + ex = examples.find { |e| e.description == "not in quarantine" } + expect(ex.execution_result.status).to eq(:passed) - ex = foo_examples.find { |e| e.description == "in quarantine" } - expect(ex.execution_result.status).to eq(:passed) + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:pending) + expect(ex.execution_result.pending_message).to eq('In quarantine') + end end - it 'if tests are not tagged they are skipped, even if they are in quarantine' do - group.run - default_context = group.children.find { |c| c.description == "default" } - default_examples = default_context.descendant_filtered_examples - expect(default_examples.count).to eq(1) - - ex = default_examples.find { |e| e.description == "in quarantine" } - expect(ex.execution_result.status).to eq(:pending) - expect(ex.execution_result.pending_message).to eq('Running tests tagged with all of [:quarantine, :foo]') + context 'in a context tagged :quarantine' do + it 'runs no tests' do + context = group.children.find { |c| c.description == "quarantine" } + expect(context.descendant_filtered_examples.count).to eq(0) + end end end - context "with 'foo' and 'bar' tagged" do + context 'with :quarantine and a non-quarantine tag (:foo) focussed' do before do RSpec.configure do |config| - config.inclusion_filter = { bar: true, foo: true } + config.inclusion_filter = { quarantine: true, foo: true } end + + group.run end after do RSpec.configure do |config| @@ -163,67 +188,67 @@ describe 'rspec config tests' do end end - it "runs tests tagged either 'foo' or 'bar'" do - group = RSpec.describe do - example 'foo', :foo do - end - example 'bar', :bar do - end - example 'foo and bar', :foo, :bar do - end - end - - group.run - expect(group.examples.count).to eq(3) + context 'in an untagged context' do + it 'ignores untagged tests and skips tests even if in quarantine' do + context = group.children.find { |c| c.description == "default" } + examples = context.descendant_filtered_examples + expect(examples.count).to eq(1) - ex = group.examples.find { |e| e.description == "foo" } - expect(ex.execution_result.status).to eq(:passed) + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:pending) + end + end - ex = group.examples.find { |e| e.description == "bar" } - expect(ex.execution_result.status).to eq(:passed) + context 'in a context tagged :foo' do + it 'only runs quarantined tests' do + context = group.children.find { |c| c.description == "foo" } + examples = context.descendant_filtered_examples + expect(examples.count).to be(2) - ex = group.examples.find { |e| e.description == "foo and bar" } - expect(ex.execution_result.status).to eq(:passed) - end + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:passed) - it "skips quarantined tests tagged 'foo' and/or 'bar'" do - group = RSpec.describe do - example 'foo in quarantine', :foo, :quarantine do - end - example 'foo and bar in quarantine', :foo, :bar, :quarantine do - end + ex = examples.find { |e| e.description == "not in quarantine" } + expect(ex.execution_result.status).to eq(:pending) end + end - group.run - expect(group.examples.count).to eq(2) + context 'in a context tagged :quarantine' do + it 'skips all tests' do + context = group.children.find { |c| c.description == "quarantine" } + examples = context.descendant_filtered_examples + expect(examples.count).to be(2) - ex = group.examples.find { |e| e.description == "foo in quarantine" } - expect(ex.execution_result.status).to eq(:pending) - expect(ex.execution_result.pending_message).to eq('In quarantine') + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:pending) - ex = group.examples.find { |e| e.description == "foo and bar in quarantine" } - expect(ex.execution_result.status).to eq(:pending) - expect(ex.execution_result.pending_message).to eq('In quarantine') + ex = examples.find { |e| e.description == "not in quarantine" } + expect(ex.execution_result.status).to eq(:pending) + end end - it "ignores quarantined tests not tagged either 'foo' or 'bar'" do - group = RSpec.describe do - example 'in quarantine', :quarantine do - end - end + context 'in a context tagged :bar and :quarantine' do + it 'skips all tests' do + context = group.children.find { |c| c.description == "quarantine" } + examples = context.descendant_filtered_examples + expect(examples.count).to be(2) - group.run + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:pending) - ex = group.examples.find { |e| e.description == "in quarantine" } - expect(ex.execution_result.status).to be_nil + ex = examples.find { |e| e.description == "not in quarantine" } + expect(ex.execution_result.status).to eq(:pending) + end end end - context "with 'foo' and 'bar' and 'quarantined' tagged" do + context 'with :quarantine and multiple non-quarantine tags focussed' do before do RSpec.configure do |config| config.inclusion_filter = { bar: true, foo: true, quarantine: true } end + + group.run end after do RSpec.configure do |config| @@ -231,34 +256,49 @@ describe 'rspec config tests' do end end - it "runs tests tagged 'quarantine' and 'foo' or 'bar'" do - group = RSpec.describe do - example 'foo', :foo do - end - example 'bar and quarantine', :bar, :quarantine do - end - example 'foo and bar', :foo, :bar do - end - example 'foo, bar, and quarantine', :foo, :bar, :quarantine do - end + context 'in a context tagged :foo' do + it 'only runs quarantined tests' do + context = group.children.find { |c| c.description == "foo" } + examples = context.descendant_filtered_examples + expect(examples.count).to be(2) + + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:passed) + + ex = examples.find { |e| e.description == "not in quarantine" } + expect(ex.execution_result.status).to eq(:pending) + expect(ex.execution_result.pending_message).to eq('Only running tests tagged with :quarantine and any of [:bar, :foo]') end + end - group.run - expect(group.examples.count).to eq(4) + context 'in a context tagged :quarantine' do + it 'skips all tests' do + context = group.children.find { |c| c.description == "quarantine" } + examples = context.descendant_filtered_examples + expect(examples.count).to be(2) - ex = group.examples.find { |e| e.description == "foo" } - expect(ex.execution_result.status).to eq(:pending) - expect(ex.execution_result.pending_message).to eq('Running tests tagged with all of [:bar, :foo, :quarantine]') + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:pending) + expect(ex.execution_result.pending_message).to eq('Only running tests tagged with :quarantine and any of [:bar, :foo]') - ex = group.examples.find { |e| e.description == "bar and quarantine" } - expect(ex.execution_result.status).to eq(:passed) + ex = examples.find { |e| e.description == "not in quarantine" } + expect(ex.execution_result.status).to eq(:pending) + expect(ex.execution_result.pending_message).to eq('Only running tests tagged with :quarantine and any of [:bar, :foo]') + end + end - ex = group.examples.find { |e| e.description == "foo and bar" } - expect(ex.execution_result.status).to eq(:pending) - expect(ex.execution_result.pending_message).to eq('Running tests tagged with all of [:bar, :foo, :quarantine]') + context 'in a context tagged :bar and :quarantine' do + it 'runs all tests' do + context = group.children.find { |c| c.description == "bar quarantine" } + examples = context.descendant_filtered_examples + expect(examples.count).to be(2) - ex = group.examples.find { |e| e.description == "foo, bar, and quarantine" } - expect(ex.execution_result.status).to eq(:passed) + ex = examples.find { |e| e.description == "in quarantine" } + expect(ex.execution_result.status).to eq(:passed) + + ex = examples.find { |e| e.description == "not in quarantine" } + expect(ex.execution_result.status).to eq(:passed) + end end end end diff --git a/spec/controllers/concerns/send_file_upload_spec.rb b/spec/controllers/concerns/send_file_upload_spec.rb index a07113a6156..cf3b24f50a3 100644 --- a/spec/controllers/concerns/send_file_upload_spec.rb +++ b/spec/controllers/concerns/send_file_upload_spec.rb @@ -52,6 +52,23 @@ describe SendFileUpload do end end + context 'with inline image' do + let(:filename) { 'test.png' } + let(:params) { { disposition: 'inline', attachment: filename } } + + it 'sends a file with inline disposition' do + # Notice the filename= is omitted from the disposition; this is because + # Rails 5 will append this header in send_file + expected_params = { + filename: 'test.png', + disposition: "inline; filename*=UTF-8''test.png" + } + expect(controller).to receive(:send_file).with(uploader.path, expected_params) + + subject + end + end + context 'with attachment' do let(:filename) { 'test.js' } let(:params) { { attachment: filename } } diff --git a/spec/fixtures/api/schemas/public_api/v4/milestone.json b/spec/fixtures/api/schemas/public_api/v4/milestone.json index 971f7980f46..6ca2e88ae91 100644 --- a/spec/fixtures/api/schemas/public_api/v4/milestone.json +++ b/spec/fixtures/api/schemas/public_api/v4/milestone.json @@ -8,7 +8,6 @@ "title": { "type": "string" }, "description": { "type": ["string", "null"] }, "state": { "type": "string" }, - "percentage_complete": { "type": "integer" }, "created_at": { "type": "date" }, "updated_at": { "type": "date" }, "start_date": { "type": "date" }, diff --git a/spec/javascripts/helpers/vuex_action_helper.js b/spec/javascripts/helpers/vuex_action_helper.js index 1972408356e..88652202a8e 100644 --- a/spec/javascripts/helpers/vuex_action_helper.js +++ b/spec/javascripts/helpers/vuex_action_helper.js @@ -84,7 +84,10 @@ export default ( done(); }; - const result = action({ commit, state, dispatch, rootState: state, rootGetters: state }, payload); + const result = action( + { commit, state, dispatch, rootState: state, rootGetters: state, getters: state }, + payload, + ); return new Promise(resolve => { setImmediate(resolve); diff --git a/spec/javascripts/ide/components/new_dropdown/modal_spec.js b/spec/javascripts/ide/components/new_dropdown/modal_spec.js index 595a2f927e9..d94cc1a8faa 100644 --- a/spec/javascripts/ide/components/new_dropdown/modal_spec.js +++ b/spec/javascripts/ide/components/new_dropdown/modal_spec.js @@ -41,6 +41,15 @@ describe('new file modal component', () => { expect(vm.$el.querySelector('.label-bold').textContent.trim()).toBe('Name'); }); + it(`${type === 'tree' ? 'does not show' : 'shows'} file templates`, () => { + const templateFilesEl = vm.$el.querySelector('.file-templates'); + if (type === 'tree') { + expect(templateFilesEl).toBeNull(); + } else { + expect(templateFilesEl instanceof Element).toBeTruthy(); + } + }); + describe('createEntryInStore', () => { it('$emits create', () => { spyOn(vm, 'createTempEntry'); diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js index 73f960dd21e..4f70570ffcc 100644 --- a/spec/javascripts/notes/stores/actions_spec.js +++ b/spec/javascripts/notes/stores/actions_spec.js @@ -1,8 +1,11 @@ import Vue from 'vue'; import $ from 'jquery'; import _ from 'underscore'; +import { TEST_HOST } from 'spec/test_constants'; import { headersInterceptor } from 'spec/helpers/vue_resource_helper'; import * as actions from '~/notes/stores/actions'; +import * as mutationTypes from '~/notes/stores/mutation_types'; +import * as notesConstants from '~/notes/constants'; import createStore from '~/notes/stores'; import mrWidgetEventHub from '~/vue_merge_request_widget/event_hub'; import testAction from '../../helpers/vuex_action_helper'; @@ -599,4 +602,139 @@ describe('Actions Notes Store', () => { ); }); }); + + describe('updateOrCreateNotes', () => { + let commit; + let dispatch; + let state; + + beforeEach(() => { + commit = jasmine.createSpy('commit'); + dispatch = jasmine.createSpy('dispatch'); + state = {}; + }); + + afterEach(() => { + commit.calls.reset(); + dispatch.calls.reset(); + }); + + it('Updates existing note', () => { + const note = { id: 1234 }; + const getters = { notesById: { 1234: note } }; + + actions.updateOrCreateNotes({ commit, state, getters, dispatch }, [note]); + + expect(commit.calls.allArgs()).toEqual([[mutationTypes.UPDATE_NOTE, note]]); + }); + + it('Creates a new note if none exisits', () => { + const note = { id: 1234 }; + const getters = { notesById: {} }; + actions.updateOrCreateNotes({ commit, state, getters, dispatch }, [note]); + + expect(commit.calls.allArgs()).toEqual([[mutationTypes.ADD_NEW_NOTE, note]]); + }); + + describe('Discussion notes', () => { + let note; + let getters; + + beforeEach(() => { + note = { id: 1234 }; + getters = { notesById: {} }; + }); + + it('Adds a reply to an existing discussion', () => { + state = { discussions: [note] }; + const discussionNote = { + ...note, + type: notesConstants.DISCUSSION_NOTE, + discussion_id: 1234, + }; + + actions.updateOrCreateNotes({ commit, state, getters, dispatch }, [discussionNote]); + + expect(commit.calls.allArgs()).toEqual([ + [mutationTypes.ADD_NEW_REPLY_TO_DISCUSSION, discussionNote], + ]); + }); + + it('fetches discussions for diff notes', () => { + state = { discussions: [], notesData: { discussionsPath: 'Hello world' } }; + const diffNote = { ...note, type: notesConstants.DIFF_NOTE, discussion_id: 1234 }; + + actions.updateOrCreateNotes({ commit, state, getters, dispatch }, [diffNote]); + + expect(dispatch.calls.allArgs()).toEqual([ + ['fetchDiscussions', { path: state.notesData.discussionsPath }], + ]); + }); + + it('Adds a new note', () => { + state = { discussions: [] }; + const discussionNote = { + ...note, + type: notesConstants.DISCUSSION_NOTE, + discussion_id: 1234, + }; + + actions.updateOrCreateNotes({ commit, state, getters, dispatch }, [discussionNote]); + + expect(commit.calls.allArgs()).toEqual([[mutationTypes.ADD_NEW_NOTE, discussionNote]]); + }); + }); + }); + + describe('replyToDiscussion', () => { + let res = { discussion: { notes: [] } }; + const payload = { endpoint: TEST_HOST, data: {} }; + const interceptor = (request, next) => { + next( + request.respondWith(JSON.stringify(res), { + status: 200, + }), + ); + }; + + beforeEach(() => { + Vue.http.interceptors.push(interceptor); + }); + + afterEach(() => { + Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor); + }); + + it('updates discussion if response contains disussion', done => { + testAction( + actions.replyToDiscussion, + payload, + { + notesById: {}, + }, + [{ type: mutationTypes.UPDATE_DISCUSSION, payload: res.discussion }], + [ + { type: 'updateMergeRequestWidget' }, + { type: 'startTaskList' }, + { type: 'updateResolvableDiscussonsCounts' }, + ], + done, + ); + }); + + it('adds a reply to a discussion', done => { + res = {}; + + testAction( + actions.replyToDiscussion, + payload, + { + notesById: {}, + }, + [{ type: mutationTypes.ADD_NEW_REPLY_TO_DISCUSSION, payload: res }], + [], + done, + ); + }); + }); }); diff --git a/spec/javascripts/notes/stores/mutation_spec.js b/spec/javascripts/notes/stores/mutation_spec.js index 4f8d3069bb5..88c40dbc5ea 100644 --- a/spec/javascripts/notes/stores/mutation_spec.js +++ b/spec/javascripts/notes/stores/mutation_spec.js @@ -527,17 +527,13 @@ describe('Notes Store mutations', () => { id: 42, individual_note: true, }; - state = { discussions: [discussion] }; + state = { convertedDisscussionIds: [] }; }); - it('toggles individual_note', () => { + it('adds a disucssion to convertedDisscussionIds', () => { mutations.CONVERT_TO_DISCUSSION(state, discussion.id); - expect(discussion.individual_note).toBe(false); - }); - - it('throws if discussion was not found', () => { - expect(() => mutations.CONVERT_TO_DISCUSSION(state, 99)).toThrow(); + expect(state.convertedDisscussionIds).toContain(discussion.id); }); }); }); diff --git a/spec/lib/banzai/filter/footnote_filter_spec.rb b/spec/lib/banzai/filter/footnote_filter_spec.rb index 2e50e4e2351..c6dcb4e46fd 100644 --- a/spec/lib/banzai/filter/footnote_filter_spec.rb +++ b/spec/lib/banzai/filter/footnote_filter_spec.rb @@ -11,6 +11,7 @@ describe Banzai::Filter::FootnoteFilter do let(:footnote) do <<~EOF <p>first<sup><a href="#fn1" id="fnref1">1</a></sup> and second<sup><a href="#fn2" id="fnref2">2</a></sup></p> + <p>same reference<sup><a href="#fn1" id="fnref1">1</a></sup></p> <ol> <li id="fn1"> <p>one <a href="#fnref1">↩</a></p> @@ -25,6 +26,7 @@ describe Banzai::Filter::FootnoteFilter do let(:filtered_footnote) do <<~EOF <p>first<sup class="footnote-ref"><a href="#fn1-#{identifier}" id="fnref1-#{identifier}">1</a></sup> and second<sup class="footnote-ref"><a href="#fn2-#{identifier}" id="fnref2-#{identifier}">2</a></sup></p> + <p>same reference<sup class="footnote-ref"><a href="#fn1-#{identifier}" id="fnref1-#{identifier}">1</a></sup></p> <section class="footnotes"><ol> <li id="fn1-#{identifier}"> <p>one <a href="#fnref1-#{identifier}" class="footnote-backref">↩</a></p> diff --git a/spec/lib/gitlab/import_export/shared_spec.rb b/spec/lib/gitlab/import_export/shared_spec.rb index f2d750c6595..2c288cff6ef 100644 --- a/spec/lib/gitlab/import_export/shared_spec.rb +++ b/spec/lib/gitlab/import_export/shared_spec.rb @@ -14,6 +14,16 @@ describe Gitlab::ImportExport::Shared do expect(subject.errors).to eq(['Error importing into [FILTERED] Permission denied @ unlink_internal - [FILTERED]']) end + it 'updates the import JID' do + import_state = create(:import_state, project: project, jid: 'jid-test') + + expect_next_instance_of(Gitlab::Import::Logger) do |logger| + expect(logger).to receive(:error).with(hash_including(import_jid: import_state.jid)) + end + + subject.error(error) + end + it 'calls the error logger with the full message' do expect(subject).to receive(:log_error).with(hash_including(message: error.message)) diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 48f1d696ff6..1645b67c329 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -311,7 +311,14 @@ describe Notes::CreateService do end it 'converts existing note to DiscussionNote' do - expect { subject }.to change { existing_note.reload.type }.from(nil).to('DiscussionNote') + expect do + existing_note + + Timecop.freeze(Time.now + 1.minute) { subject } + + existing_note.reload + end.to change { existing_note.type }.from(nil).to('DiscussionNote') + .and change { existing_note.updated_at } end end end diff --git a/spec/support/api/milestones_shared_examples.rb b/spec/support/api/milestones_shared_examples.rb index b426fadb001..5f709831ce1 100644 --- a/spec/support/api/milestones_shared_examples.rb +++ b/spec/support/api/milestones_shared_examples.rb @@ -8,17 +8,12 @@ shared_examples_for 'group and project milestones' do |route_definition| describe "GET #{route_definition}" do it 'returns milestones list' do - create(:issue, project: project, milestone: milestone) - create(:closed_issue, project: project, milestone: milestone) - create(:closed_issue, project: project, milestone: milestone) - get api(route, user) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array expect(json_response.first['title']).to eq(milestone.title) - expect(json_response.first['percentage_complete']).to eq(66) end it 'returns a 401 error if user not authenticated' do |