Skip to content
Snippets Groups Projects

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?

What are the relevant issue numbers?

Closes #48578 (closed)

Edited by Rémy Coutable

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • @dennis I left a few notes, LGTM otherwise. :thumbsup:

  • Rémy Coutable changed milestone to %11.1

    changed milestone to %11.1

  • assigned to @dennis

  • Dennis Tang added 1 commit

    added 1 commit

    Compare with previous version

  • Author Developer

    Thank you for the notes @rymai! Ready for review again.

  • assigned to @rymai

  • Dennis Tang added 1 commit

    added 1 commit

    • 36678b41 - do proper check of application settings for `allow_local_requests_from_hooks_and_services`

    Compare with previous version

  • Rémy Coutable resolved all discussions

    resolved all discussions

  • @dennis Thank you, looks good to me! :heart: :yellow_heart: :green_heart: :rocket:

  • Rémy Coutable marked the checklist item Has been reviewed by a Backend maintainer as completed

    marked the checklist item Has been reviewed by a Backend maintainer as completed

  • @tauriedavis Please review, thanks!

  • Rémy Coutable approved this merge request

    approved this merge request

  • Dennis Tang added 63 commits

    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…

    Compare with previous version

  • Author Developer

    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!

  • @dennis Yes, I think you need to check in the changes to db/schema.rb. Also, please ask for a review by someone from the database team.

  • Author Developer

    Ah! Finally, a reason to check in db/schema.rb. :smile:

    Will get a database review as well.

  • Dennis Tang changed the description

    changed the description

  • Dennis Tang added 1 commit

    added 1 commit

    Compare with previous version

  • Author Developer

    Hi @yorickpeterse, would you be able to review the database changes in this MR? Thank you in advance!

  • Just one small copy change, rest looks good to me! :star:

  • Dennis Tang added 1 commit

    added 1 commit

    Compare with previous version

  • Dennis Tang added 1 commit

    added 1 commit

    Compare with previous version

  • Taurie Davis marked the checklist item Has been reviewed by a UX Designer as completed

    marked the checklist item Has been reviewed by a UX Designer as completed

  • Dennis Tang added 1 commit

    added 1 commit

    • 173b8304 - update test after copy change

    Compare with previous version

  • Dennis Tang added 1 commit

    added 1 commit

    • 0bf5708b - Revert change on url_validator.rb

    Compare with previous version

  • Dennis Tang changed the description

    changed the description

  • Dennis Tang marked the checklist item Added the output of the migration(s) to the MR body as completed

    marked the checklist item Added the output of the migration(s) to the MR body as completed

  • Dennis Tang marked the checklist item Has been reviewed by a UX Designer as completed

    marked the checklist item Has been reviewed by a UX Designer as completed

  • Yorick Peterse approved this merge request

    approved this merge request

  • assigned to @rymai

  • Yorick Peterse marked the checklist item Has been reviewed by a Database specialist as completed

    marked the checklist item Has been reviewed by a Database specialist as completed

  • Dennis Tang added 120 commits

    added 120 commits

    Compare with previous version

  • Author Developer

    Hi @rymai, looks like this one is ready to merge! Just resolved merge conflicts. :thumbsup:

  • Rémy Coutable approved this merge request

    approved this merge request

  • Rémy Coutable marked the checklist item End-to-end tests pass (package-and-qa manual pipeline job) as completed

    marked the checklist item End-to-end tests pass (package-and-qa manual pipeline job) as completed

  • @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!

  • Author Developer

    @rymai I'll take care of that!

  • Author Developer

    @rymai to confirm, should I create a new issue or just add it to this merge request?

    • 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? :thinking:

  • Dennis Tang added 1 commit

    added 1 commit

    Compare with previous version

  • Author Developer

    Unresolved discussions count it totally wrong, I don't seem to be able to unresolve then resolve the already resolved discussion? :thinking:

    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 Tang
  • Rémy Coutable approved this merge request

    approved this merge request

  • @dennis Could you please rebase, the failure has been resolved in master now.

  • assigned to @dennis

  • Dennis Tang added 102 commits

    added 102 commits

    Compare with previous version

  • Author Developer

    @rymai Rebased!

  • assigned to @rymai

  • assigned to @dennis

  • Dennis Tang added 1 commit

    added 1 commit

    Compare with previous version

  • Author Developer

    @rymai gettext generated!

  • assigned to @rymai

  • Rémy Coutable added 71 commits

    added 71 commits

    • 26494190...f5f5a4e6 - 70 commits from branch master
    • 69ce7f52 - Merge branch 'master' into '48578-disable-gcp-free-credit-banner-at-instance-level'

    Compare with previous version

  • Rémy Coutable approved this merge request

    approved this merge request

  • Rémy Coutable marked the checklist item Documentation created/updated as completed

    marked the checklist item Documentation created/updated as completed

  • Rémy Coutable mentioned in commit b7531f71

    mentioned in commit b7531f71

  • Author Developer
  • Rémy Coutable mentioned in commit 230d1352

    mentioned in commit 230d1352

  • picked into 11-1-stable will be on 11.1.0-RC13

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading