MushroomObserver / mushroom-observer

A website for sharing observations of mushrooms.
https://mushroomobserver.org
MIT License
77 stars 25 forks source link

QR code on Field Slip show page #2068

Closed mo-nathan closed 4 months ago

mo-nathan commented 5 months ago

Mostly to demonstrate the rqrcode gem. We may want to put the QR code on this page, but mostly it's planned to get added to a PDF output for printing slips.

mo-nathan commented 5 months ago

Couple of questions that I didn't get answers this morning:

coveralls commented 5 months ago

Coverage Status

coverage: 94.395% (+0.001%) from 94.394% when pulling 4d88ad0b9967fc80edeb6d1e2405e6d46f53dec0 on njw-qr-codes into a2a9bb1d5866bb5a5c6d2f18d596f824502f72d8 on main.

JoeCohen commented 5 months ago

Suggestion: Omit the page title on field_slip#show. It duplicates what's in the displayed field slip. I don't have answers to your questions, except I don't see an easy way to do the routing you want. @nimmolo may have answers to the first two.

JoeCohen commented 5 months ago

@mo-nathan. Re routes.
I think this gets what you want, except for the:id constraints, but you can check for that in the controller. (When I added id: /.*[^\d.-].*/, the tests blow up. Without the constraint, everything passes.) In routes.rb:

  match "field_slips", to: "field_slips#update", via: [:patch, :put]
  delete("/field_slips", to: "field_slips#destroy")
  get("/qr/:id", to: "field_slips#show", as: :field_slip)
  resources :field_slips, only: [:index, :new, :create, :edit]

This yields the following routes:

% rails routes | egrep "\s/(field_slip|qr)"                               
    field_slips GET       /field_slips(.:format)           field_slips#index
                POST      /field_slips(.:format)           field_slips#create
 new_field_slip GET       /field_slips/new(.:format)       field_slips#new
edit_field_slip GET       /field_slips/:id/edit(.:format)  field_slips#edit
                PATCH|PUT /field_slips(.:format)           field_slips#update
                DELETE    /field_slips(.:format)           field_slips#destroy
     field_slip GET       /qr/:id(.:format)                field_slips#show
nimmolo commented 5 months ago

It might be better to keep a field_slip_qr_path helper separate from a field_slip_path. (Or maybe i don't yet understand why you'd want the /qr path as the default).

In any case there's no way to get Rails to generate that special path automatically with/alongside the other resources.

What I noticed in my most recent routes PR is that the named routes are used more than once, and differentiated by method, so:

If you do resources :field_slips, except: :show and try to call something else field_slip_path, it will mess up the :update path too, which you probably don't want.

mo-nathan commented 5 months ago

It might be better to keep a field_slip_qr_path helper separate from a field_slip_path. (Or maybe i don't yet understand why you'd want the /qr path as the default).

In any case there's no way to get Rails to generate that special path automatically with/alongside the other resources.

What I noticed in my most recent routes PR is that the named routes are used more than once, and differentiated by method, so:

  • field_slip_pathput is for :update and get is for :show
  • field_slips_pathpost is for :create and get is for :index.

If you do resources :field_slips, except: :show and try to call something else field_slip_path, it will mess up the :update path too, which you probably don't want.

The motivation for /qr/ rather than /field_slip/ is to make the QR code simpler. Realistically it doesn't really matter so I don't think it's worth the added complexity. Thanks for the ideas!

mo-nathan commented 5 months ago

Suggestion: Omit the page title on field_slip#show. It duplicates what's in the displayed field slip. I don't have answers to your questions, except I don't see an easy way to do the routing you want. @nimmolo may have answers to the first two.

I don't think I understand what you're getting at on field_slip#show unless it's something I pushed merged in a bit later yesterday and just merged into main from #2067. As far as I can tell the only place the field slip number is given now is in the title.

JoeCohen commented 5 months ago

@mo-nathan: Re field slip duplication of field slip number. Here's what I see (in this branch, in main, and on the webserver). Should I be seeing something different?

Screenshot 2024-03-28 at 5 27 00 AM
mo-nathan commented 5 months ago

That is what is currently in production. Did you do a pull on the branch and main? On this branch I currently get:

Screenshot 2024-03-28 at 11 21 07 AM
mo-nathan commented 5 months ago

On main I currently get:

Screenshot 2024-03-28 at 11 24 25 AM
JoeCohen commented 5 months ago

I must have been looking at the wrong branch. As you said, the FS # only appears once in #show.