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

Include sortBy parameter in BrAPI field import #902

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bellerbrock
Copy link
Collaborator

@bellerbrock bellerbrock commented Mar 1, 2024

Description

Provide a summary of your changes including motivation and context.
If these changes fix a bug or resolves a feature request, be sure to link to that issue.

Adds a spinner to choose sortBy parameter in BrAPI field imports. Sorting by studyDbId rather then studyName allows faster access to most recently created fields in a many-paged list result.

Type of change

What type of changes does your code introduce? Put an x in boxes that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated relevant documentation

Changelog entry

Please add a one-line changelog entry below. This will be copied to the changelog file during the release process.

sortBy parameter added to BrAPI field import

@trife trife requested a review from chaneylc March 8, 2024 18:55
@trife
Copy link
Member

trife commented Mar 8, 2024

I'm not sure if this is working for me. I was testing with cassavabase but the order of returned studies didn't change regardless of the choice I selected.

@bellerbrock
Copy link
Collaborator Author

Try with sweetpotatobase.org - the sortBy parameter has been part of BrAPI spec, but breedbase only just added it and I think not all their sites are updated with the latest release.

But come to think of it maybe there should be a way to test if the server has sortBy implemented, and hide the option if it isn't going to work.

Copy link
Member

@chaneylc chaneylc left a comment

Choose a reason for hiding this comment

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

My only other comment is possibly renaming the sortBy spinner options to be something more readable, but I get if we want to keep those completely brapi oriented. I thought things like: Study Name, Study ID, Trial Name, ... might be better.

@Override
public void onItemSelected(AdapterView<?> parent, View view, int position, long id) {
sortBy = parent.getItemAtPosition(position).toString();
Toast.makeText(BrapiActivity.this, "New sortBy option selected: " + sortBy, Toast.LENGTH_SHORT).show();
Copy link
Member

Choose a reason for hiding this comment

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

Your toast should use a string resource.

Copy link
Collaborator Author

@bellerbrock bellerbrock Mar 14, 2024

Choose a reason for hiding this comment

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

Good point. Was only needed for debugging anyway, so just removed it.

I don't feel too strongly either way about the names, but my thinking is that it's probably best to stick with the actual BrAPI names since they are short and should only be seen by more sophisticated users. I already get confused enough with the whole field (fieldBook) = study (BrAPI) = trial (breedbase) situation 😂

@trife
Copy link
Member

trife commented Mar 18, 2024

@bellerbrock go ahead Replace sort options with user-friendly strings. I'm also not sure if sorting by DbIds makes much sense since they're usually arbitrary. Planting/creation date or a similar timestamped field would be a good addition.

@bellerbrock
Copy link
Collaborator Author

Here's some more info from the BrAPI spec. Users can choose what they want to sort by from those options, but no date fields in there.

Which makes some sense to me because not all studies will have associated dates. But in all cases I've seen (Breedbase,T3,BIMS) studyDbId is an incrementing number that serves as the primary key in the table that stores studies, so it will sort in the same order as a create date would.

I'll switch to user-friendly strings and convert this to a draft for now. I want to talk to Pete and make a few other refinements before merging. like including sortOrder and only providing the sort option if sorting is actually implemented by the server.

@bellerbrock bellerbrock marked this pull request as draft March 19, 2024 17:11
@trife
Copy link
Member

trife commented Apr 12, 2024

@bellerbrock this can be closed, right? It looked like this was merged with the datagrid sorting

@bellerbrock
Copy link
Collaborator Author

@trife Ah, I see the confusion since they both refer to 'sort order' implementation, but no the functionality is distinct.

This PR adds sortBy and sortOrder parameters to the study GET request, while the datagrid PR added an additional attribute spinner to the BrAPI field import's preview dialog to ensure that the study is saved in Field Book with a sort order (important for cases where the obs units were being imported in random order).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants