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.

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

Merged
merged 13 commits into from
May 21, 2024
Merged

Conversation

CarolineDenis
Copy link
Contributor

@CarolineDenis CarolineDenis commented Mar 15, 2024

Fixes #4650

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add relevant issue to release milestone

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:

  • 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

@CarolineDenis CarolineDenis added this to the 7.9.6 milestone Mar 15, 2024
Copy link
Member

@maxpatiiuk maxpatiiuk left a 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:

filename = item_node.attrib['filename']

to fix this inconsistency, without breaking existing XML resources, you can simply change this line:

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

@CarolineDenis CarolineDenis marked this pull request as ready for review March 18, 2024 16:12
@CarolineDenis CarolineDenis requested a review from a team March 19, 2024 13:16
Copy link
Collaborator

@emenslin emenslin left a 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.

@grantfitzsimmons
Copy link
Member

grantfitzsimmons commented May 7, 2024

Testers– please make sure to test this on DBs that already have working export feeds configured:

  • 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

@acwhite211
Copy link
Member

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.

@CarolineDenis CarolineDenis requested review from emenslin and a team May 9, 2024 16:25
Copy link
Collaborator

@emenslin emenslin left a 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

@emenslin emenslin requested a review from a team May 13, 2024 18:23
Copy link
Collaborator

@combs-a combs-a left a 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!

@CarolineDenis CarolineDenis merged commit 51c3e8a into production May 21, 2024
9 checks passed
@CarolineDenis CarolineDenis deleted the issue-4650 branch May 21, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ExportFeed visual editor creating invalid definitions
6 participants