Remove unused tech_support_email and admin_support_email

[mayorova]

What this PR does / why we need it:

Removes unused fields from the API request and response (they are not visible in the UI and not used anywhere).

Which issue(s) this PR fixes

Closes https://issues.jboss.org/browse/THREESCALE-1615

Verification steps

tech_support_email and admin_support_email:

• are not visible in the UI
• are not read anywhere
• are not returned as part of any API response (API Service List)
• can't be added or updated in the DB

Special notes for your reviewer:

[Martouta]

If I understood correctly, these fields are not used at all anywhere.
In that case, you should remove them from everywhere, including the DB (with a migration for it) 😄

[codecov]

Codecov Report

Merging #443 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #443      +/-   ##
=========================================
+ Coverage   92.86%   92.9%   +0.03%     
=========================================
  Files        2390    2391       +1     
  Lines       77758   77851      +93     
=========================================
+ Hits        72209   72326     +117     
+ Misses       5549    5525      -24

Impacted Files	Coverage Δ
app/representers/service_representer.rb	100% <ø> (ø)	⬆️
...t/integration/user-management-api/services_test.rb	100% <ø> (ø)	⬆️
app/controllers/admin/api/services_controller.rb	100% <ø> (ø)	⬆️
lib/developer_portal/lib/liquid/drops/service.rb	82.35% <ø> (ø)	⬆️
test/integration/api/services_controller_test.rb	100% <100%> (ø)	⬆️
app/models/service.rb	93.79% <100%> (-0.05%)	⬇️
app/models/policy.rb	96.61% <0%> (-1.7%)	⬇️
app/models/payment_transaction.rb	80.64% <0%> (-1.08%)	⬇️
.../find_and_delete_accounts_scheduled_worker_test.rb	100% <0%> (ø)	⬆️
config/jobs.rb	60% <0%> (ø)	⬆️
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90284d4...a7ea2f2. Read the comment docs.

[mayorova]

@3scale/system if we could verify whether the liquid {{ admin_support_email }} is never used in SaaS without an if (it is OK in this default template I assume), I guess we can also remove https://github.com/3scale/porta/pull/443/files#diff-9a60f34aa62e2f3321db3a42f1d89fb1R184 ?

[Martouta]

According to what we spoke, this is what we need to remove from the attributes tech_support_email and admin_support_email in this specific PR:

• the attributes from the xml/json response from the model
• the attributes from the xml/json response from the representer
• update the swagger spec of the api
• fix corresponding rspec and tests.
• don't accept this params through the controllers anymore

I also see that a useful comment in lib/developer_portal/lib/liquid/drops/service.rb was added, good 👍

I will review more in depth now 😉

[Martouta]

@josemigallas You still need to change this:
https://github.com/3scale/porta/blob/48aada188a78eb65620aa22323e34222d9ac020e/app/controllers/admin/api/services_controller.rb#L113-L115
It is accepting all current attributes from service and we shouldn't allow tech_support_email or admin_support_email anymore.
Same in the other ServicesController

[Martouta]

Review done 😄 Few changes requested and that's it, you are almost there. Besides what I've said in the comments, nothing else left to do I'd say 👍

[hallelujah]

Then it should return nil and I think we should think about removing for real those deprecated tags.
@secretsurf @3scale/product @3scale/support
We need to make a plan on when and how to remove those.

My idea would be in 3 steps (3 releases)

1. add a logger there to know if people are still using it. And collect data.
2. Send a deprecation notice to those tenants
3. Remove the feature definitively

[josemigallas]

@hallelujah @macejmic
Should we add here as well 1 for the tech_support_email?

hidden
deprecated "Use **support_email** instead."
def tech_support_email
  @service.support_email
end

[alanmoran]

@hallelujah I would be in favour of add a logger so we can collect usage data and go from there.

[Martouta]

So I guess this should be like the following example (please confirm @hallelujah ):

# NOTE: keeping in case somebody already using 'admin_support_email' liquid
hidden
deprecated "Use **support_email** instead."
def admin_support_email
  Rails.logger.info "Liquid::Drops::Service#admin_support_email is being used by Service ##{@service.id}"
  nil
end

[Martouta]

@josemigallas No need to add the tech_support_email here 😄 If it isn't there is because nobody is using it which is what we want 😄

[hallelujah]

👍 Sorry this method do not have to change, forget about my first comment.

[Martouta]

Good 👍 💪

Doing a new review now :)

[Martouta]

Great job @josemigallas 🎉 🥇

[hallelujah]

Remove the attr_readonly and good to

[Martouta]

LGTM 👍
About https://github.com/3scale/porta/pull/443/files#r271680214, it is either what you did there or strong parameters in the controllers instead of accepting everything from the exterior:
https://github.com/3scale/porta/blob/48aada188a78eb65620aa22323e34222d9ac020e/app/controllers/admin/api/services_controller.rb#L113-L115
https://github.com/3scale/porta/blob/48aada188a78eb65620aa22323e34222d9ac020e/app/controllers/api/services_controller.rb#L37
https://github.com/3scale/porta/blob/48aada188a78eb65620aa22323e34222d9ac020e/app/controllers/api/services_controller.rb#L52

Personally I prefer the strong params option instead of the readonly for this, but I understand that it is more risky and scary 😜

[hallelujah]

Ok then prepare a separate PR to remove the columns then

[ianseyer]

It would've been great to get a public release notice about this, as it broke our production APIs 👎

[thomasmaas]

Hi @ianseyer, that's very unfortunate. We are investigating what went wrong. Would you care to explain how this change broke your production APIs?

[thomasmaas]

ping @ianseyer: to get some more specifics would be good.