Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✂️✂️✂️ Remove API v0 support ✂️✂️✂️ #19062

Closed
wants to merge 5 commits into from

Conversation

fdocr
Copy link
Contributor

@fdocr fdocr commented Feb 3, 2023

What type of PR is this? (check all applicable)

  • Refactor
  • Optimization

Description

After the transition period and now that the API V1 docs are ready we're in shape to get rid of the old V0 API endpoints and stick with the new major version going forward.

This PR gets rid of the concerns extraction of logic (used to avoid repeating the code on shared controllers) so it will all be easier to find/understand in the codebase.

Related Tickets & Documents

N/A

QA Instructions, Screenshots, Recordings

  • Check out the code locally and try out some API endpoints
  • Check out the specs in place and ensure they're ✅

UI accessibility concerns?

None - backend change

Added/updated tests?

  • Yes

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

  • I've updated the Developer Docs or
    Storybook (for Crayons components)
  • This PR changes the Forem platform and our documentation needs to be
    updated. I have filled out the
    Changes Requested
    issue template so Community Success can help update the Admin Docs
    appropriately.
  • I've updated the README or added inline documentation
  • I've added an entry to
    CHANGELOG.md
  • I will share this change in a Changelog
    or in a forem.dev post
  • I will share this change internally with the appropriate teams
  • I'm not sure how best to communicate this change and need help
  • This change does not need to be communicated, and this is why not: please
    replace this line with details on why this change doesn't need to be
    shared

[optional] Are there any post deployment tasks we need to perform?

This is a sensitive change. I strongly recommend some manual monitoring over the next hour or two after this is merged to ensure no edge case scenarios are causing an elevated amount of user facing errors

[optional] What gif best describes this PR or how it makes you feel?

monkey pulling bunny out of brain "tada!"

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@fdocr fdocr changed the title Remove API v0 support ✂️✂️✂️ Remove API v0 support ✂️✂️✂️ Feb 3, 2023
@fdocr fdocr added the merge by any core This tag is a signal that you are okay with any core team member merging your code for you. label Feb 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

Uffizzi Preview Environment deployment-13456

☁️ https://app.uffizzi.com/github.com/forem/forem/pull/19062

📄 View Application Logs etc.

What is Uffizzi? Learn more

@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Base: 80.17% // Head: 79.96% // Decreases project coverage by -0.21% ⚠️

Coverage data is based on head (1fbde09) compared to base (605ebbf).
Patch coverage: 98.21% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #19062      +/-   ##
==========================================
- Coverage   80.17%   79.96%   -0.21%     
==========================================
  Files        1136     1102      -34     
  Lines       25012    24835     -177     
  Branches     1758     1769      +11     
==========================================
- Hits        20054    19860     -194     
- Misses       4709     4723      +14     
- Partials      249      252       +3     
Flag Coverage Δ
javascript 46.35% <ø> (+0.05%) ⬆️
ruby 93.78% <98.21%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/controllers/application_controller.rb 95.38% <ø> (ø)
app/controllers/api/v1/users_controller.rb 93.33% <92.30%> (-6.67%) ⬇️
app/controllers/api/v1/articles_controller.rb 95.89% <95.45%> (-4.11%) ⬇️
app/controllers/api/v1/analytics_controller.rb 97.67% <97.14%> (-2.33%) ⬇️
app/controllers/api/v1/admin/users_controller.rb 100.00% <100.00%> (ø)
app/controllers/api/v1/comments_controller.rb 100.00% <100.00%> (ø)
app/controllers/api/v1/feature_flags_controller.rb 100.00% <100.00%> (ø)
app/controllers/api/v1/followers_controller.rb 100.00% <100.00%> (ø)
app/controllers/api/v1/follows_controller.rb 100.00% <100.00%> (ø)
app/controllers/api/v1/health_checks_controller.rb 100.00% <100.00%> (ø)
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines -5 to +10
include Api::Admin::UsersController

before_action :authenticate!
before_action :authorize_super_admin

def create
# NOTE: We can add an inviting user here, e.g. User.invite!(current_user, user_params).
User.invite!(user_params)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what you'll find in all v1 controllers: Removing the include of the concern and inclusion of the concern code itself in the controller

@fdocr fdocr marked this pull request as ready for review February 3, 2023 20:36
@fdocr fdocr requested review from a team as code owners February 3, 2023 20:36
@fdocr fdocr requested review from maestromac, jaw6 and Ridhwana and removed request for a team February 3, 2023 20:36
@benhalpern benhalpern added blocked blocked by internal or external issues and removed merge by any core This tag is a signal that you are okay with any core team member merging your code for you. labels Feb 7, 2023
@maestromac maestromac marked this pull request as draft February 7, 2023 21:34
@Ridhwana Ridhwana removed their request for review March 16, 2023 11:23
@maestromac maestromac removed their request for review May 8, 2023 16:14
@mirie
Copy link
Contributor

mirie commented Aug 3, 2023

I will close this for now. When we decide how to move forward, we can open with a new PR

@mirie mirie closed this Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked blocked by internal or external issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants