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

Field Slips #2024

Merged
merged 61 commits into from
Mar 26, 2024
Merged

Field Slips #2024

merged 61 commits into from
Mar 26, 2024

Conversation

mo-nathan
Copy link
Member

@mo-nathan mo-nathan commented Mar 12, 2024

This provides the key functionality needed for Field Slips. See #1947 for the issue and #1896 for a more detailed discussion.

This PR delivers the functionality where there is a route that accepts a field slip code. The /qr/:code route is provided for simplicity and brevity. To test you probably want to update some projects with field slip prefixes. For example, if you're working with a recent database dump you could add the prefix '6F' to the First Fantastic Fungal Friends Foray Festival! or 'NFAL' to North Falmouth Fungi. You can then exercise the new functionality with URLs like:

localhost:3000/qr/6F-0001
localhost:3000/qr/NFAL-0001

The #1947 has a checklist of functionality that I added as part of the PR to get to the point where it feels like a cohesive whole.

If you want to try it starting from a QR code, you can create QR codes here: https://www.the-qrcode-generator.com/

@mo-nathan mo-nathan requested a review from nimmolo March 12, 2024 11:51
config/locales/en.txt Outdated Show resolved Hide resolved
@mo-nathan
Copy link
Member Author

@nimmolo Thanks for the comments. Happy to follow your suggestions. Most of the code is actually from rails generate scaffold. I was just trying to get something working as a starting point. Did you see my comment in #general in Slack? I'm trying to figure out how to make the matrix_box in app/views/field_slips/_field_slip.html.erb look reasonable instead of ultra skinny. I'm guessing there's some reasonably simple way to do that. I played around with it a bit but I'm hopeless with front end stuff.

@JoeCohen
Copy link
Member

I pushed a short (failing) integration test exposing the bug. You can run it with:
rails t test/integration/capybara/field_slips_integration_test.rb
Sorry I didn't get any further.

If -- without using Field Slips -- I create an Observation, fill in the fields with something that violates project constraints, and click Create,
it:

  • redisplays the create_observation form with a flash warning ("This observation violates the constraints of one or more selected projects")
  • does NOT create an Observation until I again click Create.

But if I scan a field slip, click Create New Observation, fill in constraint-violating info, then

  • it redisplays the create_observation form, gives me the flash warning,
  • BUT immediately creates an Observation (apparently including the FieldSlip, but ignoring namings, images).

@mo-nathan
Copy link
Member Author

mo-nathan commented Mar 24, 2024

To do:

  • Fix double observation bug
  • Fix titles (include a rename for the "New field slip" header)
  • Fix system tests

@mo-nathan
Copy link
Member Author

localhost:3000/qr/6F-0001 Last Observation is a surprise. I expected it to show my last Observation. Instead it's showing the Obs at the top of the Activity Feed, which:

  • was created 2013-06-23,
  • updated 2024-03-20 06:34:14 PDT (-0700)
  • created by Vlad Lekach (laddycans)
  • for which I proposed a Name
  • is the local copy of https://mushroomobserver.org/137413

From checking through the new method in ObservationView#last_observation, I believe @mo-nathan has it pulling the last viewed observation by the current user, and that may not be the expected obs.

This is a really interesting case. I can see an argument for limiting it to observations the current user created, but I think there is a legitimate use case where someone like a project admin might want to create a field slip object that the user creating the observation didn't know to create. I'm hoping the use of the last observation feature is more of a rare power user thing and if they legitimately want to create a field slip for someone else's observation I think they should have a way to do that (e.g., a project admin sees an observation that should have been included that has a photo of the field slip, but the field slip link is on the observation). Maybe I should add a comment on the page making it clearer that this is the last observation you viewed and if you view another observation in another window and reload the page or rescan the same QR code after visiting the observation you care about, then it will update. Does that seem reasonable?

@mo-nathan
Copy link
Member Author

mo-nathan commented Mar 24, 2024

I pushed a short (failing) integration test exposing the bug. You can run it with: rails t test/integration/capybara/field_slips_integration_test.rb Sorry I didn't get any further.

If -- without using Field Slips -- I create an Observation, fill in the fields with something that violates project constraints, and click Create, it:

  • redisplays the create_observation form with a flash warning ("This observation violates the constraints of one or more selected projects")
  • does NOT create an Observation until I again click Create.

But if I scan a field slip, click Create New Observation, fill in constraint-violating info, then

  • it redisplays the create_observation form, gives me the flash warning,
  • BUT immediately creates an Observation (apparently including the FieldSlip, but ignoring namings, images).

Thanks for the test! Turns out the fix was quite simple. I was ignoring the success flag. I also need to pass the field_code param to the next page.

@coveralls
Copy link
Collaborator

coveralls commented Mar 24, 2024

Coverage Status

coverage: 94.394% (+0.02%) from 94.372%
when pulling 246e7d4 on njw-field-slips
into dcffad0 on main.

@JoeCohen
Copy link
Member

JoeCohen commented Mar 24, 2024

I banged on field slips for a while.
A possible issue: inconsistent relation among FieldSlip, Observation, Project.
Here's what I did:

Screenshot 2024-03-24 at 10 07 42 AM

Result:

  • Created a FieldSlip, correctly relating it to the Observation and the Project:
Screenshot 2024-03-24 at 10 12 24 AM
  • Updated the Observation, adding the FieldSlip #, but not the Project:
Screenshot 2024-03-24 at 10 18 17 AM
  • Did not add the Observation to the Project.

@nimmolo
Copy link
Contributor

nimmolo commented Mar 24, 2024

Attempted some improved markup for the form on #2063 - it's a PR off this branch

@mo-nathan
Copy link
Member Author

I banged on field slips for a while. A possible issue: inconsistent relation among FieldSlip, Observation, Project. Here's what I did:

Screenshot 2024-03-24 at 10 07 42 AM * Created an Observation, **which was not in the Project** * Pasted http://localhost:3000/qr/SARCO-0001 into the address bar, selected Last Observation (the one I just created).

Result:

  • Created a FieldSlip, correctly relating it to the Observation and the Project:
Screenshot 2024-03-24 at 10 12 24 AM * Updated the Observation, adding the FieldSlip #, but not the Project: Screenshot 2024-03-24 at 10 18 17 AM * Did not add the Observation to the Project.

Interesting case. I see what you're driving at. Howevver, I am imagining that observations associated with field slips that are not associated with the same project to be reasonable common in the long run. The case I'm thinking of are folks who go to events that are using MO field slips and walk away with a bunch of them. In this case it seems reasonable to associate the field slip with the project (due to the prefix) and this could be useful once we have forms associated with projects, but if the observation doesn't meet the constraints of the project, then it's legit for it not to be added. I guess it would be fine to add it to the project if it meets the contraints, but not if it doesn't with appropriate flash messages.

@mo-nathan
Copy link
Member Author

Resolved last observation getting added to project.

@mo-nathan mo-nathan merged commit a2a9bb1 into main Mar 26, 2024
5 checks passed
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.

None yet

4 participants