Resolve "Disable GCP free credit banner at instance level"
What does this MR do?
Adds the option to hide third-party offers in the Admin -> Application Settings section.
If this option is left unchecked, i.e., display third party offers, the user will still be able to opt-out of it once they click the "x" on the offer, as it functioned before.
Are there points in the code the reviewer needs to double check?
Nope.
Why was this MR needed?
See issue.
Screenshots (if relevant)
Database Checklist
When adding migrations:
-
Updated db/schema.rb
-
Added a down
method so the migration can be reverted -
Added the output of the migration(s) to the MR body
== 20180704204006 AddHideThirdPartyOffersToApplicationSettings: migrating =====
-- transaction_open?()
-> 0.0000s
-- execute("SET statement_timeout TO 0")
-> 0.0004s
-- transaction()
-- add_column(:application_settings, :hide_third_party_offers, :boolean, {:default=>nil})
-> 0.0026s
-- change_column_default(:application_settings, :hide_third_party_offers, false)
-> 0.0076s
-> 0.0117s
-- transaction_open?()
-> 0.0000s
-- exec_query("SELECT COUNT(*) AS count FROM \"application_settings\"")
-> 0.0007s
-- exec_query("SELECT \"application_settings\".\"id\" FROM \"application_settings\" ORDER BY \"application_settings\".\"id\" ASC LIMIT 1")
-> 0.0004s
-- exec_query("SELECT \"application_settings\".\"id\" FROM \"application_settings\" WHERE \"application_settings\".\"id\" >= 1 ORDER BY \"application_settings\".\"id\" ASC LIMIT 1 OFFSET 1")
-> 0.0005s
-- execute("UPDATE \"application_settings\" SET \"hide_third_party_offers\" = 'f' WHERE \"application_settings\".\"id\" >= 1")
-> 0.0012s
-- change_column_null(:application_settings, :hide_third_party_offers, false)
-> 0.0013s
== 20180704204006 AddHideThirdPartyOffersToApplicationSettings: migrated (0.0228s)
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated -
API support added -
Tests added for this feature/bug - Conform by the code review guidelines
-
Has been reviewed by a UX Designer -
Has been reviewed by a Frontend maintainer -
Has been reviewed by a Backend maintainer -
Has been reviewed by a Database specialist
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Conform by the database guides -
If you have multiple commits, please combine them into a few logically organized commits by squashing them -
Internationalization required/considered -
End-to-end tests pass ( package-and-qa
manual pipeline job)
What are the relevant issue numbers?
Closes #48578 (closed)
Merge request reports
Activity
added 1 commit
- e79bd347 - add option to hide third party offers in admin application settings
assigned to @dennis
marked the checklist item Changelog entry added, if necessary as completed
marked the checklist item Conform by the merge request performance guides as completed
marked the checklist item Conform by the style guides as completed
marked the checklist item Conform by the database guides as completed
marked the checklist item If you have multiple commits, please combine them into a few logically organized commits by squashing them as completed
Hi @rymai, would you be available to review this? It should be a quick one!
@tauriedavis this is ready for your review as well!
assigned to @rymai
- Resolved by Rémy Coutable
- Resolved by Rémy Coutable
- Resolved by Rémy Coutable
- Resolved by Rémy Coutable
- Resolved by Rémy Coutable
@dennis I left a few notes, LGTM otherwise.
changed milestone to %11.1
assigned to @dennis
Thank you for the notes @rymai! Ready for review again.
assigned to @rymai
added 1 commit
- 36678b41 - do proper check of application settings for `allow_local_requests_from_hooks_and_services`
@dennis Thank you, looks good to me!
@tauriedavis Please review, thanks!
assigned to @tauriedavis
added 63 commits
-
36678b41...2510985f - 61 commits from branch
master
- cf5c5dfb - Merge remote-tracking branch 'origin/master' into…
- 05bfeb21 - Merge branch '48578-disable-gcp-free-credit-banner-at-instance-level' of…
-
36678b41...2510985f - 61 commits from branch
Hi @rymai, do I need to do anything about these failing tests and schema jobs? I'll create the EE port to resolve
ee_compat_check
, but I'm not sure what I should do about the others. Thank you!Ah! Finally, a reason to check in
db/schema.rb
.Will get a database review as well.
Hi @yorickpeterse, would you be able to review the database changes in this MR? Thank you in advance!
assigned to @yorickpeterse
- Resolved by Rémy Coutable
This MR has a corresponding EE MR: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6395
Thanks @dennis!
mentioned in issue gitlab-com/database#116 (closed)
assigned to @rymai
added 120 commits
-
0bf5708b...34fe3274 - 119 commits from branch
master
- b70f41da - Merge remote-tracking branch 'origin/master' into…
-
0bf5708b...34fe3274 - 119 commits from branch
Hi @rymai, looks like this one is ready to merge! Just resolved merge conflicts.
@dennis It looks like me still miss documentation, could you please create an issue and write the documentation before the final 11.1 release? Thanks!
@rymai I'll take care of that!
@rymai to confirm, should I create a new issue or just add it to this merge request?
- Resolved by Rémy Coutable
- Resolved by Rémy Coutable
to confirm, should I create a new issue or just add it to this merge request?
As you prefer, I think the feature freeze didn't happen yet, but since documentation don't need an exception request, it might be better to merge this MR ASAP, and then you can contribute the documentation later. ;)
@dennis Unresolved discussions count it totally wrong, I don't seem to be able to unresolve then resolve the already resolved discussion?
Unresolved discussions count it totally wrong, I don't seem to be able to unresolve then resolve the already resolved discussion?
That's an issue with the MR vue refactor, I believe. I'll see if I can find the issue for it.
EDIT: I think this is it: https://gitlab.com/gitlab-org/gitlab-ce/issues/48803
Edited by Dennis Tangmentioned in issue gitlab-org/release/tasks#317 (closed)
@dennis Could you please rebase, the failure has been resolved in
master
now.assigned to @dennis
added 102 commits
-
091cf5b7...d4301731 - 92 commits from branch
master
- 67ae7ec4 - add option to hide third party offers in admin application settings
- 8f30ed63 - update tests
- 7eac0277 - revisions
- 67ba5946 - do proper check of application settings for `allow_local_requests_from_hooks_and_services`
- 89b8c2c6 - commit db/schema.rb
- 36e2d298 - copy fix
- eac6041d - update schema version
- 1fb2017a - update test after copy change
- 59889798 - Revert change on url_validator.rb
- 2289d136 - add documentation
Toggle commit list-
091cf5b7...d4301731 - 92 commits from branch
@rymai Rebased!
assigned to @rymai
assigned to @dennis
@rymai gettext generated!
assigned to @rymai
added 71 commits
-
26494190...f5f5a4e6 - 70 commits from branch
master
- 69ce7f52 - Merge branch 'master' into '48578-disable-gcp-free-credit-banner-at-instance-level'
-
26494190...f5f5a4e6 - 70 commits from branch
Omnibus doesn't seem to want to build the package... Let's see https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6395#note_87061521 instead.
marked the checklist item Documentation created/updated as completed
mentioned in commit b7531f71
Exception approved in gitlab-org/release/tasks#317 (closed)
mentioned in commit 230d1352