Restore no-targets welcome message, add Messier option. #1171

Fingel · 2025-02-04T06:06:04Z

This PR addresses two things:

Restores the message in the target list table when there are no targets yet in the database, pointing the user to add a target manually or add one from a broker. This functionality was broken presumably at some point when the table became an inclusion tag.
Add an option to import the Messier catalog, a quick and easy option for someone who has just set up a TOM and wants to explore it's functionality quickly.

This PR addresses two things: 1. Restores the message in the target list table when there are no targets yet in the database, pointing the user to add a target manually or add one from a broker. This functionality was broken presumably at some point when the table became an inclusion tag. 2. Add an option to import the Messier catalog, a quick and easy option for someone who has just set up a TOM and wants to explore it's functionality quickly.

jchate6

Very nice!
A few comments.

jchate6 · 2025-02-04T19:38:20Z

tom_targets/templates/tom_targets/partials/target_table.html

@@ -43,8 +43,14 @@
    <tr>
      <td colspan="5">
        {% if target_count == 0 and not query_string %}
-        No targets yet. You might want to <a href="{% url 'tom_targets:create' %}">create a target manually</a>
-        or <a href="{% url 'tom_alerts:list' %}">import one from an alert broker</a>.
+        There are no targets in the database.


This can be a bit misleading if a user doesn't have permissions to see the targets in the DB.
If a logged out user clicks on this when targets are in the DB, it will prompt a login then the page will crash.

Method Not Allowed (GET): /targets/seed/ Method Not Allowed (GET): /targets/seed/ Method Not Allowed: /targets/seed/ Method Not Allowed: /targets/seed/

This is pretty confusing. I created a second, non-admin user and when I login, I can't see any targets. I create a target on my admin user, and it's in the public group, still I can't see anything on the non-admin user. I had to explicitly create a group, add the new user to the group, and add those targets to that group to see them on any non-admin user. Is this intentionally how it's supposed to work?

I would have thought that by default targets would be added to the inferred public group. But that doesn't seem to be the case - I can actually import the messier objects as a non-admin user but can't see them unless I log in as an admin.

How should these seed objects be created so that non-admin users can see them?

OK I figured it out. It's because the user must have the tom_targets.Target.view_target permission:

tom_base/tom_targets/views.py

Line 74 in 4a46922

permission_required = f'{Target._meta.app_label}.view_target'

This doesn't seem like it's set when a user is created by an admin in the user section, but maybe it should be? I don't see this mentioned anywhere in the docs. If I wasn't already familiar with the Django permissions system and knew I could set it in /admin I don't think I'd have been able to figure this out as easily.

So, the permissions structure is complicated because different toms have different needs. We can't really dictate who gets to see what.

That being said, by default, a non-admin user SHOULD be able to see everything in the public group, but AnonymousUser should not. By default the public group doesn't have access to new targets. This includes the Messier objects.

if I go and update them individually to give the public user group access (Target detail page --> update) then I can see them just fine from my non-admin user.
However, if I delete them and try to create them again via the button, from this test user, they are created, but they aren't created with permissions such that either the public group, or even that specific test user has access.

The issue I mentioned above is actually with regards to the AnonymousUser. If you log out and go to the target list page, and try to add messier objects, it will prompt a login as you try to pass through targets/seed, and give a 405. We probably should hide the create button for the AnonymousUser. or have the logic:
if no targets, and no login --> display "Login to see targets"

In this case, I think it doesn't matter if public is given access by default.

A TOM developer might have their TOM permissions configured in such a way that there may be Targets in the db, but they're just not visible to the current user. So, I'm not sure the toolkit itself can, without more logic, claim that there are no Targets in the database.

@jchate6 I see what you're saying. Latest commit will check if the user is authenticated.

A TOM developer might have their TOM permissions configured in such a way that there may be Targets in the db, but they're just not visible to the current user. So, I'm not sure the toolkit itself can, without more logic, claim that there are no Targets in the database.

I added this logic.

tom_targets/seed.py

phycodurus

The idea behind this issue

Management command to load some Targets #1170

was to put the add-messier-catalog code snippet into a form that was more broadly useful (i.e. a flexible management command that users could use to add targets). With use cases like:

./manage.py addtarget Vega
./manage.py addtarget --catalog NED NCG 1234 which could query NED, while the default catalog is SIMBAD. I'm fine with just searching one catalog at a time, but one suggestion was query multiple catalogs in some order until the queried object is found (but this seems overly complicated to me)
./manage.py addtarget --messier which is how the code snippet might be packaged up.

I think this turns the messier.py code snippet, which is nice for testing by devs to a useful feature for users.

phycodurus · 2025-02-04T22:56:25Z

tom_targets/seed.py

+]
+
+
+def seed_messier_targets():


This function is really about creating Targets, the context (that we're seeding the db) isn't really the point and I'd like not to introduce a new concept (i.e."seeding") without a stronger reason. So, can we rename the function to something like create_messier_targets? and I'm not sure what to call the module right now.

phycodurus · 2025-02-04T23:19:39Z

tom_targets/seed.py

@@ -0,0 +1,121 @@
+from tom_targets.models import Target, TargetList
+
+messier_targets = [


I'm on the fence about single-source-of-truth vs. network-is-slow-and-don't-hammer-SIMBAD arguments.

If we consider the code as someplace a TOM developer might look to as an example of how one might add a Target with an alias, the logic of: I queried SIMBAD and the result came back with a different name than I queried with, so add that as an additional TargetName motivates and demonstrates that.

Also, messier.py provides an example of how to add a target_extra for Object Type, which I kind of liked.

phycodurus · 2025-02-04T23:24:05Z

tom_targets/templates/tom_targets/partials/target_table.html

@@ -43,8 +43,14 @@
    <tr>
      <td colspan="5">
        {% if target_count == 0 and not query_string %}
-        No targets yet. You might want to <a href="{% url 'tom_targets:create' %}">create a target manually</a>
-        or <a href="{% url 'tom_alerts:list' %}">import one from an alert broker</a>.
+        There are no targets in the database.


A TOM developer might have their TOM permissions configured in such a way that there may be Targets in the db, but they're just not visible to the current user. So, I'm not sure the toolkit itself can, without more logic, claim that there are no Targets in the database.

phycodurus · 2025-02-04T23:25:40Z

tom_targets/urls.py

@@ -25,6 +26,7 @@
    path('create/', TargetCreateView.as_view(), name='create'),
    path('import/', TargetImportView.as_view(), name='import'),
    path('export/', TargetExportView.as_view(), name='export'),
+    path('seed/', TargetSeedView.as_view(), name='seed'),


see above comment wrt seed

Fingel · 2025-02-05T00:09:59Z

@phycodurus

The idea here stems from my experience as a "new tom user" yesterday. I spun up a new TOM, and was presented with an empty table that said "No targets match those filters". I tried to use a broker to add some targets, it's not intuitive and I couldn't get any of them to work in a few minutes. Manual creation is easy but feels bad if you don't have a RA/DEC memorized of a real target.

So the idea is to provide an option for a small set of usable targets immediately, if the user wants them, as well as to point them to the other methods of adding targets.

I would call that seeding. I don't really care what or how we do it. If we want to use a broker like SIMBAD, or we want to use the NGC catalog, or something else entirely, I don't think it matters - these targets are in most cases all going to be deleted anyway. All that matters if we give the user something, anything, that actually exposes the functionality of the TOM immediately.

Also in regard to management commands, I would like to understand the reasoning better for adding more functionality with them. I wouldn't call management commands user-friendly. In fact in some deployment scenarios, users won't be able to use them at all. So that's why I avoided adding this as a management command.

jchate6

One optional request, but otherwise good to merge.

jchate6 · 2025-02-06T00:26:33Z

tom_targets/templates/tom_targets/partials/target_table.html

+                You might want to <a href="{% url 'tom_targets:create' %}">create a target manually</a>,
+                <a href="{% url 'tom_alerts:list' %}">import one from an alert broker</a>
+                or
+                <form method="POST" action="{% url 'tom_targets:seed' %}" class="d-inline">


Can you add a link for this to the tom_targets.templates.tom_tagets.partials.recent_targets.html partial used on the index page after the "create a target" link?

I think if our goal is to quickly and obviously populate a tom with targets, this would be a great spot to do that from.

Good idea, added.

Fingel · 2025-02-06T17:46:33Z

Added tests, and the links to help create targets to the recent targets partial.

Fingel linked an issue Feb 4, 2025 that may be closed by this pull request

Management command to load some Targets #1170

Closed

Fingel requested a review from phycodurus February 4, 2025 06:06

jchate6 self-requested a review February 4, 2025 16:25

jchate6 requested changes Feb 4, 2025

View reviewed changes

Explicitly name what types of targets eare being seeded

cf0486b

phycodurus reviewed Feb 4, 2025

View reviewed changes

Check if DB is actually empty, and proper auth redirect

2931b34

phycodurus approved these changes Feb 5, 2025

View reviewed changes

jchate6 approved these changes Feb 6, 2025

View reviewed changes

Fingel added 3 commits February 6, 2025 09:28

Add target creation hints to recent target list

8001193

Add seeded targets to public group

48844bb

Add tests and redirect on GET

c505fef

Merge branch 'dev' into 1170-management-command-to-load-some-targets

4f3305c

Fingel merged commit 08fed00 into dev Feb 6, 2025
22 checks passed

Fingel deleted the 1170-management-command-to-load-some-targets branch February 6, 2025 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore no-targets welcome message, add Messier option. #1171

Restore no-targets welcome message, add Messier option. #1171

Fingel commented Feb 4, 2025

jchate6 left a comment

jchate6 Feb 4, 2025

Fingel Feb 4, 2025

Fingel Feb 4, 2025

jchate6 Feb 4, 2025

jchate6 Feb 4, 2025

phycodurus Feb 4, 2025

Fingel Feb 4, 2025

Fingel Feb 5, 2025

phycodurus left a comment

phycodurus Feb 4, 2025

phycodurus Feb 4, 2025

phycodurus Feb 4, 2025

phycodurus Feb 4, 2025

Fingel commented Feb 5, 2025

jchate6 left a comment

jchate6 Feb 6, 2025

Fingel Feb 6, 2025

Fingel commented Feb 6, 2025

		@@ -0,0 +1,121 @@
		from tom_targets.models import Target, TargetList

		messier_targets = [

		]


		def seed_messier_targets():

Restore no-targets welcome message, add Messier option. #1171

Restore no-targets welcome message, add Messier option. #1171

Conversation

Fingel commented Feb 4, 2025

jchate6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phycodurus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fingel commented Feb 5, 2025

jchate6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fingel commented Feb 6, 2025