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.

Sign up for GitHub

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

Jump to bottom

Cleanup static files and fix related issues #5312

Closed
40 tasks done
CarolineDenis opened this issue Oct 4, 2024 · 2 comments
Closed
40 tasks done

Cleanup static files and fix related issues #5312

CarolineDenis opened this issue Oct 4, 2024 · 2 comments
Labels
1 - Request Improvements or extensions to existing behavior type:housekeeping Code cleanup and refactoring
Milestone
7.9.x

Comments

@CarolineDenis
Copy link
Contributor

CarolineDenis commented Oct 4, 2024 •
edited by grantfitzsimmons
Loading

  1. Existing issues:
  1. Cleanup duplicates
    Besides that, since there is a lot of duplication for each discipline, I've only left a single comment for each type of app resource in a single discipline. Any comments on a type of app resource should apply anywhere it appears.
    If there are no comments on a type of app resource, that means I found usages of the app resource in Specify 7 and it should be kept.
  • config/accessions/app_resources.xml
    I actually don't think Specify 7 accesses any file in the accessions directory.
    It looks like it is supposed to be a type of discipline, but even if a discipline has the accessions type, it's not defined in the DISCIPLINE_DIRS:

specify7/specifyweb/context/app_resource.py

Lines 26 to 41 in 2a0b516

DISCIPLINE_DIRS = {
"fish": "fish",
"herpetology": "herpetology",
"paleobotany": "invertpaleo",
"invertpaleo": "invertpaleo",
"vertpaleo": "vertpaleo",
"bird": "bird",
"mammal": "mammal",
"insect": "insect",
"botany": "botany",
"invertebrate": "invertebrate",
"minerals": "minerals",
"anthropology": "anthropology",
# "vascplant": "vascplant",
# "fungi": "fungi",
}

So it will never be accessed when determining the file path:

specify7/specifyweb/context/app_resource.py

Lines 88 to 103 in 2a0b516

def get_path_for_level(collection, user, level):
"""Build the filesystem path for a given resource level."""
discipline_dir = None if collection is None else \
DISCIPLINE_DIRS.get(collection.discipline.type, None)
usertype = get_usertype(user)
paths = {
'UserType' : (discipline_dir, usertype) if discipline_dir and usertype else None,
'Discipline': (discipline_dir,) if discipline_dir else None,
'Common' : ('common' ,),
'Backstop' : ('backstop' ,)}
path = paths.get(level, None)
if path:
return os.path.join(settings.SPECIFY_CONFIG_DIR, *path)

  • config/backstop/collstats.xml
    This file shouldn't be needed: Specify 7 has its own implementation for statistics.
    Although, people can still access this file using /context/app.resource?name=CollStats, so keep it if you want 🤷
  1. Shouldn't be needed:

I'm pretty sure this file shouldn't be needed either.
I couldn't find any references to this file (even with its "human readable" name: DialogDefs) except in a static test file on the frontend.

Looks like Specify 6 uses this to define the respective Java classes for all Dialogs which are used in the App:
https://github.com/specify/specify6/blob/master/src/edu/ku/brc/specify/ui/DBObjDialogFactory.java
(Something that Specify 7 doesn't care about)

Further, for all views in Specify 7, Specify accesses the viewset_registry.xml in the level directory and iterates over all entries in the registry:

specify7/specifyweb/context/viewsets.py

Lines 38 to 46 in 2a0b516

matches = ((id, viewset, view, src, level)
# db first, then disk
for get_viewsets, src in ((get_viewsets_from_db, 'db'), (load_viewsets, 'disk'))
# then by directory level
for level in AR.DIR_LEVELS
# then in the viewset files in a given directory level
for id, viewset in get_viewsets(collection, user, level)
# finally in the list of views in the file
for view in viewset.findall(xpath))

specify7/specifyweb/context/viewsets.py

Lines 124 to 144 in 2a0b516

def load_viewsets(collection, user, level):
"""Try to get a viewset for a given level and context from the filesystem."""
# The directory structure for viewsets are the same as for app resources.
path = AR.get_path_for_level(collection, user, level)
if not path: return []
# The viewset registry lists all the viewset files for that directory.
registry = AR.load_registry(path, 'viewset_registry.xml')
if registry is None: return []
# Load them all.
def viewsets():
for f in registry.findall('file'):
try:
file_name = f.attrib['file']
relative_path = os.path.join(os.path.relpath(path, settings.SPECIFY_CONFIG_DIR),file_name)
yield relative_path, get_viewset_from_file(path, file_name)
except Exception:
pass
return viewsets()

So most of the time, Specify 7 is directly getting the search views from search.views.xml in backstop:

specify7/config/backstop/viewset_registry.xml

Line 5 in 8534551

<file type="system" name="Search" title="Search" file="search.views.xml"/>

This specific view file is commented out in the backstop viewset registry, so it will never be used currently:

specify7/config/backstop/viewset_registry.xml

Line 6 in 8534551

<!--file type="system" name="gbif" title="gbif" file="gbif.views.xml" /-->

Additionally, the GBIFCollection will not work as a Specify 7 form
It also has a similar definition in global.views.xml which Specify 7 can not use, and so can be removed

Same thing as CollStats and similar views in the backstop viewset registry: this viewset is used to create the forms for preferences in Specify 6.

See the Java files in the below directory for references - you can search for the string createForm("Preferences -
https://github.com/specify/specify6/tree/master/src/edu/ku/brc/specify/prefs

Specify 7 doesn't really care about this file then, as it has its own implementation of preferences.

All StartUpPanel files (startup_panel.xml in the app resource registries) are not used or accessed by Specify 7

All StatisticsPanel files (statistics_panel.xml in the app resource registries) are not used or accessed by Specify 7

All StatsSummaryPanel (stats_summary_panel.xml in the app resource registries) are never used or accessed by Specify 7.

All /picklist.xml files are unused by Specify 7.

Specify 7 also doesn't use the /queryextralist.xml (aka, QueryExtraList).

Specify 7 also does not use the /queryfreqlist.xml (aka, QueryFreqList).

Specify 7 does not use any of the /show_fields.xml files.

Specify 7 doesn't use any of the /startup_panel.xml files (aka StartUpPanel).

Same thing with the other statistics related files: Specify 7 does not use any of the /statistics_panel.xml files.

Specify 7 doesn't use any of the /stats_summary_panel.xml files (StatsSummaryPanel).

Specify 7 doesn't use any of the /taxon_init.xml files.

  1. change the path of the config directory:

Looks like @maxpatiiuk already started integrating these schema_localization.xml files with Weblate:

https://github.com/specify/specify7/tree/production/specifyweb/frontend/js_src/lib/localization/schema-localization

Though now that we have the files directly in the Specify 7 repository, maybe we can change the path of the config directory which lot of that code uses:

specify7/specifyweb/frontend/js_src/lib/localization/schema-localization/pull.ts

Lines 27 to 30 in 2a0b516

.requiredOption(
'-c, --config-directory <string>',
'Location of the "config" directory in Specify 6 repository'
)

From the analyze of @melton-jason on pr #5266
@CarolineDenis CarolineDenis added the 1 - Request Improvements or extensions to existing behavior label Oct 4, 2024
@CarolineDenis CarolineDenis added this to the 7.9.x milestone Oct 4, 2024
@grantfitzsimmons grantfitzsimmons added the type:housekeeping Code cleanup and refactoring label Oct 4, 2024
@maxpatiiuk
Copy link
Member

maxpatiiuk commented Oct 5, 2024 •
edited
Loading

Is this issue going to be resolved in the same PR or a separate PR?
I would strongly encourage to delete the useless files in the PR that added them and then squash-merge the PR so as not to pollute our git history with needless files.
The history in the Specify 6 repo was quite polluted, and as a result the repository was quite slow (in extreme example, someone committed a 100mb database file into git!) - one of the first things Ben did at Specify was to clean up the git history of sp6 - let's not have to do the same in sp7

@grantfitzsimmons grantfitzsimmons mentioned this issue Oct 7, 2024
Closed
@grantfitzsimmons
Copy link
Member

grantfitzsimmons commented Nov 7, 2024 •
edited
Loading

Fixed in #5367

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
1 - Request Improvements or extensions to existing behavior type:housekeeping Code cleanup and refactoring
Projects
None yet
Milestone
7.9.x
Development

No branches or pull requests
3 participants
@grantfitzsimmons @maxpatiiuk @CarolineDenis