Skip to content

1396 update reduced datums for existing target via data service#1426

Open
jchate6 wants to merge 22 commits intodevfrom
1396-update-reduced-datums-for-existing-target-via-data-service
Open

1396 update reduced datums for existing target via data service#1426
jchate6 wants to merge 22 commits intodevfrom
1396-update-reduced-datums-for-existing-target-via-data-service

Conversation

@jchate6
Copy link
Contributor

@jchate6 jchate6 commented Feb 25, 2026

Adds Redundancy for new data from Data Services
Adds Docs
Adds form to Target detail page to Target Detail page to get new data from Data Service.

@jchate6 jchate6 requested a review from Fingel February 25, 2026 22:03
@jchate6 jchate6 linked an issue Feb 25, 2026 that may be closed by this pull request
9 tasks
@jchate6 jchate6 removed the request for review from Fingel February 25, 2026 22:03
@jchate6 jchate6 moved this to Needs Review in TOM Toolkit Feb 25, 2026
@jchate6 jchate6 requested review from Fingel and phycodurus February 25, 2026 23:36
"""Builds the query parameters from the form data"""
raise NotImplementedError(f'build_query_parameters method has not been implemented for {self.name}')

# Include this method if you wish for the TOM to be able to query data for an individual Target.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm looking for advice here. I'm using the existence of this method in forms.py:69 to test whether something should be in the form choices. I'm curious if there is a better way to do this.
This way seems imperfect because it doesn't show up in the docs now and breaks the symmetry for other methods being used here.

@Fingel Fingel mentioned this pull request Feb 26, 2026
Copy link
Contributor

@Fingel Fingel left a comment

Choose a reason for hiding this comment

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

This is nice. Having all the code related to a single service in one module is compelling.

Having spent the time working on the Alerce implementation, I would say this is good but there is one area I think improvements could be made: making the code more self-documenting.

I spent a lot of time trying to figure out what various function parameters and return values were or were supposed to be. I resorted to a lot of print() debugging for parameters like query_parmeters target, target_result, query_results and data. Are they dictionaries? Objects? If they are dictionaries, what keys do they have? There was a lot of tracing function calls to understand how the data flowed.

The documentation might cover these calls in english, but it's really helpful to have the feedback directly in the code/editor as well. There are a few things that might be considered:

Use actual objects where it makes sense instead of dictionaries. If an object doesn't make sense because it would have no methods, use a dataclass (designed for this very purpose). At the least, type annotations would go a long way to at least differentiate between objects and dictionaries.

That's it. Nice work!

Copy link
Member

@phycodurus phycodurus left a comment

Choose a reason for hiding this comment

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

One other comment we talked about was an <hr> above the version (and maybe one between the Form and the "Run/Save Query" row).



.. code-block:: html
:caption: my_dataservice/partials/myservice_simple_form.html
Copy link
Member

Choose a reason for hiding this comment

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

We talked about double-checking that the filename :caption:(s) are correct.

Also, the agreement between these /path/to/partial return values and where the partials are in the file system (and how they are named (extention or no)) might be worth an explicit mention or trouble-shooting tip.


Simple forms are often a single field that will find expected results. Such as a Target name or ID field.

This consists of adding the ``get_simple_form_partial()`` method to MyDataService and then creating the partial.
Copy link
Member

Choose a reason for hiding this comment

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

These methods fall more under the responsibility of the Form class.

(This refactor would affect the get_context_data() method of the View where they are used).

@jchate6
Copy link
Contributor Author

jchate6 commented Mar 5, 2026

Refactor:

  • Add "simple_form_field" tag to form fields
  • Automatically include/exclude "simple_form_fields" from appropriate forms
  • Move partial definitions into form
  • Move Target save logic from view into to_target
  • Update Docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

Update reduced Datums for existing Target via data service

3 participants