tethysplatform / tethys

The Tethys Platform main Django website project repository.
http://tethysplatform.org/
BSD 2-Clause "Simplified" License
94 stars 51 forks source link

Paths API #1037

Closed sdc50 closed 6 months ago

sdc50 commented 7 months ago

Initial implementation of Paths API current tests passing

re #1031

TODO:

sdc50 commented 7 months ago

@gagelarsen Here is my branch for the Paths API. I believe it's all functional, but would like to do more testing. Current tests are passing, but new coverage is needed to test the code additions. Also, documentation needs to be added/updated.

I've marked some places in the code were we need to deprecate the Workspaces API, but I haven't added any deprecation warnings.

coveralls commented 7 months ago

Coverage Status

coverage: 100.0%. remained the same when pulling d97d29ef02fd36e3ea82fee41bdfd5dcc4fe636c on paths_api into bcb73599964463b5da921b3cea8bf8fe25924f7c on main.

gagelarsen commented 7 months ago

@sdc50 I have written most of the tests. There are a few lines left to cover, but I wasn't sure the best way to hit them. Could you take a look? The rest is ready for you to do documentation and deprecations

gagelarsen commented 7 months ago

@sdc50 not sure what to do with the consumer_with_paths method in testing. Figured you could take a look at it.

swainn commented 7 months ago

@sdc50 not sure what to do with the consumer_with_paths method in testing. Figured you could take a look at it.

There is also a TODO to integrate it with the TethysConsumer classes

sdc50 commented 7 months ago

@swainn / @shawncrawley As I'm writing docs for the Paths API I realized that the with_workspaces decorator (used with handers) is part of the Routing API and in tethys_sdk.base (though undocumented), but it was not in the Workspaces API. Should we add the new with_paths decorator to these same APIs and the Paths API? or should it just be added to the Paths API? or should we not add it to any API and keep it as an internal tool for implementing the with_paths argument to the handers API?

swainn commented 7 months ago

@swainn / @shawncrawley As I'm writing docs for the Paths API I realized that the with_workspaces decorator (used with handers) is part of the Routing API and in tethys_sdk.base (though undocumented), but it was not in the Workspaces API. Should we add the new with_paths decorator to these same APIs and the Paths API? or should it just be added to the Paths API? or should we not add it to any API and keep it as an internal tool for implementing the with_paths argument to the handers API?

Don't add new paths decorators, but add the paths arguments to the controller decorator. Deprecate the workspace decorators as discussed in #1031.

sdc50 commented 7 months ago

@swainn How should we handle workspace clearing with the Paths API? Should there be a separate way to clear the media directory, or should the current method clear both the workspace and the media directories?

I just pushed up the docs that I've written. If you could review (primarily the workspaces API and Paths API) them and let me know if they seem complete.

I've added a TODO list in the description of the PR with the remaining docs to update.

sdc50 commented 7 months ago

Should there be a separate way to clear the media directory, or should the current method clear both the workspace and the media directories?

I added pre_delete_*_media and post_delete_*_media methods to the TethysAppBase class, so they can be handled separately from the workspaces, but I just call them from the same views that clear the workspace (i.e. there is not a separate interface in the admin/user profile to delete the media).

sdc50 commented 6 months ago

@swainn Thanks for reviewing. I've addressed all of your comments and believe this is ready to go now.