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

Removed unused code related to the v1 & Intacct API #4293

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

wes-otf
Copy link
Contributor

@wes-otf wes-otf commented Dec 17, 2024

Removes unused/unmaintained code that was pertaining to the previous React frontend along with the attempted Intacct API integrations. The only RESTful endpoint left is /api/v1/rounds/open, which (when utilizing an API key created in Django Admin) will return a JSON output of all available rounds.

Test Steps

  • Ensure Hypha works as it did before (especially in regards to invoicing, as that's where most of the Intacct/deliverable code was actually implemented)
  • Confirm the /api/v1/rounds/open endpoint works as it did before

@wes-otf wes-otf added the Type: Enhancement This is an improvement of an existing thing (not a new thing, which would be a feature). label Dec 17, 2024
@wes-otf
Copy link
Contributor Author

wes-otf commented Dec 17, 2024

wouldn't be hard to toss django-rest-framework altogether in this scenario as we talked about on Monday but wasn't sure if we wanted to do a manual API Key implementation or if there would be any additional APIs in the future that would need some framework.

also added some documentation but if we do end up adding a lot more endpoints in the future it may be smart to leverage swagger or something

@wes-otf
Copy link
Contributor Author

wes-otf commented Dec 17, 2024

unpublished rounds also show up as open in the endpoint output because of #3867, which will probably be my next focus

@theskumar
Copy link
Member

It would be great to handle the removal of drf-dependency in another PR. Manual API key is not too hard. We can use django-ninja, but I don't see the need for it now, simple JSONResponse would do.

Regarding docs, simple Markdown-based docs will work best. That allows you to document any rate limit, authentication, and sample requests.

# def get_context_data(self, **kwargs):
# invoice = self.get_object()
# project = invoice.project
# return super().get_context_data(**kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Remove this completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah great catch! I originally intended to, slipped by me

@frjo
Copy link
Contributor

frjo commented Dec 18, 2024

I think we should remove it all.

The "rounds/open" end point can be rebuilt and token is likely overkill for that endpoint. Just rate limiting it should be enough, the content you get is the same that is displayed on the front page in any case.

@frjo
Copy link
Contributor

frjo commented Dec 18, 2024

A lot of work went it to this code but since we are not using it it's good to get out of the way.

I do not know of anyone using the "rounds/open" endpoint.

@frjo frjo added the Type: Minor Minor change, used in release drafter label Dec 18, 2024
@wes-otf
Copy link
Contributor Author

wes-otf commented Dec 18, 2024

I tossed the endpoint, could definitely see the utility of one like it in the future though

@frjo
Copy link
Contributor

frjo commented Dec 18, 2024

@wes-otf Do a uv remove djangorestframework-api-key djangorestframework drf-nested-routers (I think they are not needed anymore.)

That will update pyproject.toml and uv.lock.

Then attempt to commit and the pre-commit hook will notice that the requirements/*.txt needs updating and do that. Then you commit again and push. (or run pre-commit run --all-files before you commit.)

@wes-otf wes-otf force-pushed the maintenance/remove-old-api-integrations branch from 2408680 to 5fcf095 Compare December 23, 2024 16:01
@wes-otf
Copy link
Contributor Author

wes-otf commented Dec 23, 2024

@frjo thanks for that pointer - still learning the uv ropes! I believe all should be good to go now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement This is an improvement of an existing thing (not a new thing, which would be a feature). Type: Minor Minor change, used in release drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants