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

Prevent sorting by '-audio' citation and lexeme forms #1807

Merged
merged 4 commits into from
May 16, 2024

Conversation

imnasnainaec
Copy link
Contributor

@imnasnainaec imnasnainaec commented May 2, 2024

Fixes issue where words are sorted by audio filenames if the first analysis writing system is an audio one.

Description

The default sort uses the first lexeme form (if no citation form is present) which is a problem if the first analysis writing system is an audio writing system. Then the entries are sorted by the filename, which results in a meaningless sort. This pr prevents '-audio' citation forms and lexeme forms from being used.

Checklist

  • I have labeled my PR with: bug, feature, engineering, security fix or testing
  • I have performed a self-review of my own code
  • I have reviewed the title & description of this PR which I will use as the squashed PR commit message
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have enabled auto-merge (optional)

Testing

Testers, use the following instructions against our staging environment. Post your findings as a comment and include any meaningful screenshots, etc.

Describe how to verify your changes and provide any necessary test data.

  • Test A

@imnasnainaec
Copy link
Contributor Author

Needs "bug" label added.

@hahn-kev hahn-kev requested a review from megahirt May 2, 2024 16:12
@hahn-kev hahn-kev added the bug An existing problem with our app in production label May 2, 2024
@hahn-kev hahn-kev requested a review from rmunn May 2, 2024 16:15
Copy link
Collaborator

@megahirt megahirt left a comment

Choose a reason for hiding this comment

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

Thanks for this Danny! I really appreciate the help and fix. I don't think I would have had time to dig into this until the summer.

@megahirt
Copy link
Collaborator

megahirt commented May 3, 2024

@imnasnainaec note that the two failing e2e tests on Firefox (passes on Chrome) cropped after our npm package upgrade. We haven't figured out why but it's documented I'm not worried and it doesn't have anything to do with your changes.

@megahirt
Copy link
Collaborator

megahirt commented May 3, 2024

@imnasnainaec if you are ready for this to be merged, go ahead and mark it as ready for review and I'll take care to get it published.

@imnasnainaec imnasnainaec marked this pull request as ready for review May 3, 2024 13:40
@imnasnainaec imnasnainaec changed the title Prevent sorting by '-audio' lexeme forms Prevent sorting by '-audio' citation and lexeme forms May 3, 2024
@imnasnainaec
Copy link
Contributor Author

imnasnainaec commented May 3, 2024

@megahirt The one thing I've not done is describe how testers can test this change. When I tried to create a project with an audio writing system as the first analysis writing system, it didn't come up as first in Language Forge.

Copy link
Collaborator

@rmunn rmunn left a comment

Choose a reason for hiding this comment

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

Nice work; thanks.

Copy link
Collaborator

@megahirt megahirt left a comment

Choose a reason for hiding this comment

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

new changes look good to me

@rmunn
Copy link
Collaborator

rmunn commented May 13, 2024

Just merged #1809 into develop, so now I'm merging develop into this PR so that the E2E tests will go green again.

@rmunn rmunn merged commit 9479df3 into sillsdev:develop May 16, 2024
16 checks passed
@imnasnainaec imnasnainaec deleted the getSortableValue branch June 4, 2024 13:02
@megahirt
Copy link
Collaborator

This PR just shipped and I have verified that https://languageforge.org/app/lexicon/5c0ffc0d8b21470bcf09155f sorts properly now with @imnasnainaec 's fix. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An existing problem with our app in production
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants