-
Notifications
You must be signed in to change notification settings - Fork 38
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
Fix camel case issue for field in export feed #4660
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #4650 (comment)
Instead of adding an exeption on the front-end, we should fix this on the back-end because this is the only place across all XML files where we have camelCase XML attributes rather than lowercase or kebab-case
even worse, some of the attributes in the same XML file are lowercase and some camelCase, which is very inconsistent:
specify7/specifyweb/export/feed.py
Line 39 in f216dae
filename = item_node.attrib['filename'] |
to fix this inconsistency, without breaking existing XML resources, you can simply change this line:
specify7/specifyweb/export/feed.py
Line 45 in f216dae
collection = Collection.objects.get(id=item_node.attrib['collectionId']) |
to
collection = Collection.objects.get(id=item_node.get('collectionid', item_node.get('collectionId')))
(the above will try to get collectionid
attribute if it exists, and if not, fallback to
and same for the other 2
specifyweb/frontend/js_src/lib/components/Syncer/fromSimpleXmlNode.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing instructions
-
delete any existing export feed
-
save
-
create a new export feed using the visual editor
-
switch to xml editor
-
verify the collectionId and userId are camel case
-
verify that you can do an export
collectionId and userId are not camel case however I was able to export anyway. Is this the new expected behavior? And if so then the first check for testing instructions should be removed.
Testers– please make sure to test this on DBs that already have working export feeds configured:
|
I tested with and without camelCase for the RSS Export Feed on the KUFish database, and both seem to work fine. Let me know if anyone finds a case or specific database where it is not working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing instructions
-
delete any existing export feed
-
save
-
create a new export feed using the visual editor
-
switch to xml editor
-
verify the collectionId and userId are camel case
-
verify that you can do an export
also test:
- Navigate to a database with an existing export feed (one created before this PR)
- Verify that the export works in
edge
- Update the RSS feed from the User Tools menu
- Verify that the export is completed successfully in
issue-4650
works both with and without camel case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing instructions
-
delete any existing export feed
-
save
-
create a new export feed using the visual editor
-
switch to xml editor
-
verify the collectionId and userId are camel case
-
verify that you can do an export
Also worked with or without camel case for me, was all lowercase by default.
- Navigate to a database with an existing export feed (one created before this PR)
- Verify that the export works in
edge
- Update the RSS feed from the User Tools menu
- Verify that the export is completed successfully in
issue-4650
Worked perfectly, used sross on herb_rbge_2_5_2024. I had some issues with it on edge, where it'd take awhile to load, but it seems to work better on this PR? And same as above--it went to lowercase, but it seemed to work just fine regardless of camel case or not.
Also stumbled into some unrelated issues related to RSS feeds in general but overall it seems like this is working! Good job!
Fixes #4650
Checklist
and self-explanatory (or properly documented)
Testing instructions
delete any existing export feed
save
create a new export feed using the visual editor
switch to xml editor
verify the collectionId and userId are camel case
verify that you can do an export
@grantfitzsimmons edit, also test:
edge
issue-4650