ElektraInitiative / PermaplanT

https://www.permaplant.net
BSD 3-Clause "New" or "Revised" License
16 stars 13 forks source link

seeds entry #35

Closed markus2330 closed 1 year ago

markus2330 commented 1 year ago

We entered:

{"message":"ok","data":[{"id":1,"name":"Tomate","variety_id":1,"harvest_year":2023,"quantity":"Enough","tags":["Fruit crops"],"use_by":null,"origin":null,"taste":null,"yield_":null,"generation":null,"quality":null,"price":null,"notes":null},{"id":2,"name":"Tomate","variety_id":1,"harvest_year":2022,"quantity":"Enough","tags":["Fruit crops"],"use_by":null,"origin":"Arche Noah","taste":null,"yield_":null,"generation":null,"quality":"Organic","price":null,"notes":"Für Salat und zum Kochen geeignet."},{"id":3,"name":"Mangold","variety_id":1,"harvest_year":2021,"quantity":"More than enough","tags":["Leaf crops"],"use_by":"2025-01-26","origin":"Stanzach","taste":"lecker :-)","yield_":"1","generation":-2,"quality":"Not organic","price":null,"notes":""},{"id":4,"name":"Spinat","variety_id":1,"harvest_year":2018,"quantity":"Not enough","tags":["Leaf crops"],"use_by":"2021-01-01","origin":"Kiepenkerl","taste":null,"yield_":null,"generation":0,"quality":"Organic","price":null,"notes":"schnellwachsend & widerstandsfähig"}]}
buenaflor commented 1 year ago

Sorte: auto-selection should be based on Art chosen before

Is there any ruleset behind this that we can follow? how can we infer the "Sorte" if Art is a free text that can have any random input or do you mean the "Kategorie"?

Sorte: it should be possible to also enter own text

Which column in the seeds table would that be stored into? Currently we only have the variety_id to reference a specific Sorte

via network I got when going to "New Entry" "AxiosError: Network Error" (probably it tries to connect to wrong backend 127.0.0.1? I tried with npm run dev -- --host 0.0.0.0) http://x.x.x.x:5173/seeds -> no network error but it also does not list seeds

Currently the backend only allows requests from localhost:5173 or 127.0.0.1:5173 due to cors, see backend but this is something we have to change anyway as soon as we deploy it. Preferrably we add a field to the .env so we can configure it

browser back button and then abort: cannot press yes

abort button confirmation is currently still in todo since I was unsure from which site we can enter the seed entry form, but generally we can just navigate to the previous route

markus2330 commented 1 year ago

for overview: bezugsjahr, art, sorte, herkunft, (edited at)

markus2330 commented 1 year ago

how can we infer the "Sorte" if Art is a free text that can have any random input or do you mean the "Kategorie"?

  1. (optional) The user first changes "Kategorie". This has influence which "Art" are selectable.
  2. The user chooses an "Art". This has influence which "Sorte" is selectable.
  3. Either:
    • The "Sorte" is selected. Then this seed will refer to this "Sorte". "Sorte" of seed stays NULL. OR
    • Some Text is written in "Sorte". Then this seed will refer to the "Art" without "Sorte". The written "Sorte" is the written text.
  4. (optional) "Kategorie" is changed, this gets stored in "Seed". If another "Art" is selected, "Kategorie" gets resetted.

Please ask if anything is unclear.

markus2330 commented 1 year ago

I have to revert my previous algorithm. It is too complicated and the needed data about Kategorie is not in the database.

So let us simplify it and:

@buenaflor note that #42 completely redefines the varieties table (it is currently called plant_details there). So we need to coordinate together to get this release done. Maybe we should have a short meeting in the beginning of the week to clarify table names etc. of the database.

badnames commented 1 year ago

Which column in the seeds table would that be stored into? Currently we only have the variety_id to reference a specific Sorte

Variety, i.e., Sorte later will be select-able from database but for the first milestone it is simply text to be entered (As I assume we will not be able to implement the "Edit Plants" use case next week, which would be needed to add the varieties.)

@buenaflor note that https://github.com/ElektraInitiative/PermaplanT/pull/42 completely redefines the varieties table (it is currently called plant_details there). So we need to coordinate together to get this release done. Maybe we should have a short meeting in the beginning of the week to clarify table names etc. of the database.

I'm currently working on this issue. Should I add a VARCHAR "variety" to the database in place of variety_id for now?

markus2330 commented 1 year ago
markus2330 commented 1 year ago

I added

in the top field. They occurred when testing #77.

markus2330 commented 1 year ago

I added

You can fill up the database by running "npm run insert data/detail.csv" in scraper.

markus2330 commented 1 year ago

@badnames @buenaflor Which points are currently being worked on? Where do you need help?

markus2330 commented 1 year ago

Paul will look into the network errors.

badnames commented 1 year ago

Which points are currently being worked on? Where do you need help?

@buenaflor Told me he is working on the seeds page.

I will be taking on these two minor tasks:

English/German mixture (should be everything English with translations)

date: it should be also possible to only enter year (if day/month is unknown) Otherwise date entry is very fancy

If there is some time left in my schedule, I may also start working on this issue:

actually use plants from database

markus2330 commented 1 year ago

Looks great, we are coming to the end spurt. I added:

@Bushuo can you take this over?

Bushuo commented 1 year ago

@markus2330 Sure, I will do that in coordination with @badnames

badnames commented 1 year ago

date: it should be also possible to only enter year (if day/month is unknown) Otherwise date entry is very fancy

@markus2330 I've been working on this yesterday and today. The more I look into it, the more I doubt that this feature will be worth the effort.

First of, we need to find some way of storing the chosen date resolution (day, month or year) in the database. This by extension will translate to added code complexity in the backend and thus more effort when implementing new Features that make use of the "best by" date in the future.

Furthermore designing an appropriate UI component, that is not too confusing or overly complicated is not a trivial task.

Please don't misunderstand me, this feature is certainly doable within this week. However, I also think the time needed to do this correctly and the added complexity is not worth such a tiny improvement.

markus2330 commented 1 year ago

@badnames I agree, let us stay with the current solution. Much more important is that we get the data from the plants when creating seeds.

badnames commented 1 year ago

I just talked to @Bushuo. We came to the conclusion that existing plants can already be accessed in the seed form. However, we also noticed that pagination has not been implemented yet.

Since this would be a major performance issue if the database is filled with scraped data (causing megabytes of unnecessary data to be transmitted on page load), we will now focus on this new issue.

markus2330 commented 1 year ago

Did you already try to import all data from the scraper and see if you can search for plants (for both species and variety)? At least for variety I am quite sure it is not yet implemented: As already said it should be additionally possible to have Seeds of variety not present in the database. But if the wanted variety is in the plant table, then variety in seeds table should be NULL and the seed should directly refer to the variety.

Bushuo commented 1 year ago

Hi @markus2330, we are a little bit at a loss here. I will summarise, the best I can according to my current knowledge. Currently there is a plants and a plant_detail table (was called varieties in an earlier version) in the database. plants.plant_type references plant_detail.id and plant_detail is populated by the scraper.

What is the purpose of the plants table?

Currently the dropdown Variety in the seeds/new form is populated by the entries of the plants table. Is this correct? If yes, is there a way to populate the plants table (even manually)?

markus2330 commented 1 year ago

What is the purpose of the plants table?

It contains all species and some varieties plants that seeds might be from.

Currently the dropdown Variety in the seeds/new form is populated by the entries of the plants table. Is this correct?

Yes, both species and varieties should be populated by the plants table. (is_variety will tell you how to distinguish)

If yes, is there a way to populate the plants table (even manually)?

Did you look at the scraper/README.md? Was there an error when running npm run insert?

You can ask @aidnurs about both plants table and the scraper (he is the author).

Bushuo commented 1 year ago

@markus2330 Thanks, for the quick answer.

Did you look at the scraper/README.md? Was there an error when running npm run insert?

Yes, I read it. There was no error, and plant_detail is filled up. But plants just contains the dummy entry Wildtomate.

Currently the dropdown Variety in the seeds/new form is populated by the entries of the plants table. Is this correct?

Yes, both species and varieties should be populated by the plants table. (is_variety will tell you how to distinguish)

Where is this field is_variety? I don't see it in the current data schema.

badnames commented 1 year ago

Yes, I read it. There was no error, and plant_detail is filled up. But plants just contains the dummy entry Wildtomate.

I have added a temporary SQL script in our branch, that moves the data from plant_detail to plants. This should be integrated into the existing scraper before we merge tough.

Yes, both species and varieties should be populated by the plants table. (is_variety will tell you how to distinguish)

Are we already supposed to convert the variety field back to a plants table reference?

markus2330 commented 1 year ago

Where is this field is_variety? I don't see it in the current data schema.

Ok, now I understand where you confusion comes from, I am now also confused. In "scraper/data/detail.csv" there is a "Is Variety" but it seems like it does not get mapped to the plants in the database?

So probably the code that inserts the data is incomplete. But with the proposal below this does not matter anymore as variety and plants are basically treated identical in the search field, so you don't need is_variety.

Are we already supposed to convert the variety field back to a plants table reference?

Obviously our old proposal is too complicated (as there were already several such confusions by different people). So let us simplify again. There should be two fields:

  1. The plant search field (former species): here you can search for all plants, including all varieties, all German common names, all English common names and all synonyms. Custom text is not possible, you must select something from the plants database.
  2. The additional name (former variety): this is a custom text that the user can add to specify the specific seed the user has.

Example 1: The user searches for "Brokkoli" in plant search field (which is the German common name for a variety), and then can write Purple Sprouting as additional name.

Example 2: The user searches for "Tomato" in plant search field (which is the English common name for a species) and then can write Glühbirnchen as additional name (which would be the variety in this case).

The huge advantage of this simplification is that this plant search field will be the identical as we will use in the plant layer usecase. (It has a small downside if there are many varieties, like for pumpkin or tomatoes, but this should be fixable with a clever ranking algorithm that offers "normal" pumpkin or tomatoes as first option.)

badnames commented 1 year ago

So just to make sure what needs to be done now: is there any purpose left in having a seperate plants table or can we safely remove it? As far as I'm aware plant_detail contains all columns that are currently available in plants.

markus2330 commented 1 year ago

I think this is an optimization. It was done by @aidnurs, so it is best if he tells us. I agree that something like this should be documented, there is the issue #97 but this particular piece (documenting tables) maybe is better done next to the SQL code?

@badnames what do you think? Where were you searching for information what plant_detail could be?

badnames commented 1 year ago

Ok, thank you for the answer. Then we will keep the table for now.

I was searching in the SQL migrations and in issues/PRs.

badnames commented 1 year ago

@markus2330 Can you please confirm that the remaining issues from your inital comment have now been fixed? Because otherwise we should be done with this issue as soon as the integration of plant_detail is merged! :fireworks: :champagne: :partying_face:

markus2330 commented 1 year ago

cannot change "Menge" or "Qualität" via cursor

still doesn't work. What I mean is: going with tab to the field, and then pressing up and down should change the entry. (It might be actually okay and only visual problem that I don't see what is active).

  • [ ] browser back button discards all entered data

also doesn't work, should work like "Cancel" works

Then a few renames/simplifications/reorderings:

everything is also updated in top-post.

Bushuo commented 1 year ago

@badnames @markus2330 I would take a look at this later today.

Bushuo commented 1 year ago

still doesn't work. What I mean is: going with tab to the field, and then pressing up and down should change the entry. (It might be actually okay and only visual problem that I don't see what is active).

It was indeed a visibility problem. I fixed the styles on our branch @badnames

markus2330 commented 1 year ago

Great job, only a few items left! :rocket:

markus2330 commented 1 year ago

How is progress? Are there further problems?

Bushuo commented 1 year ago

@markus2330 Working on the frontend right now. I have a problem with one specific query and looking into it. Somehow searching for Tom always errors out.

Bushuo commented 1 year ago

~~I can not figure it out... This seems to be an error in diesel, but no useful error value is provided. Literally, every other 3 character search query works...~~ I logged the query with

let debug = diesel::debug_query::<diesel::pg::Pg, _>(&query);
println!("\nQuery: {}", debug);

Using the psql client the same query works. Directly and as a prepared statement. The generated query looks like this

SELECT "plants"."id", "plants"."binomial_name", "plants"."common_name", "plants"."common_name_de", "plants"."family", "plants"."subfamily", "plants"."genus", "plants"."edible_uses", "plants"."medicinal_uses", "plants"."material_uses_and_functions", "plants"."botanic", "plants"."propagation", "plants"."cultivation", "plants"."environment", "plants"."material_uses", "plants"."functions", "plants"."provides_forage_for", "plants"."provides_shelter_for", "plants"."hardiness_zone", "plants"."heat_zone", "plants"."water", "plants"."sun", "plants"."shade", "plants"."soil_ph", "plants"."soil_texture", "plants"."soil_water_retention", "plants"."environmental_tolerances", "plants"."native_climate_zones", "plants"."adapted_climate_zones", "plants"."native_geographical_range", "plants"."native_environment", "plants"."ecosystem_niche", "plants"."root_zone_tendancy", "plants"."deciduous_or_evergreen", "plants"."herbaceous_or_woody", "plants"."life_cycle", "plants"."growth_rate", "plants"."mature_size_height", "plants"."mature_size_width", "plants"."fertility", "plants"."pollinators", "plants"."flower_colour", "plants"."flower_type", "plants"."created_at", "plants"."updated_at", "plants"."has_drought_tolerance", "plants"."tolerates_wind", "plants"."plant_references", "plants"."is_tree", "plants"."nutrition_demand", "plants"."preferable_permaculture_zone", "plants"."article_last_modified_at" FROM "plants" WHERE UPPER(binomial_name) LIKE $1 OR UPPER(ARRAY_TO_STRING(common_name, ', ')) LIKE $2 LIMIT $3 -- binds: ["%TOM%", "%TOM%", 10]

EDIT: Was our enum conversion.

Bushuo commented 1 year ago

browser back button discards all entered data

@markus2330 In modern browsers it is not possible to display custom content in the beforeunload popup anymore. Should we use the default popup?

Screenshot 2023-03-30 at 23 22 41
markus2330 commented 1 year ago

Great, good to see that you are on it! :heart:

I logged the query with

It is usually best if you show the full code in a PR, then it is easier to understand what you did and it can even be debugged.

EDIT: Was our enum conversion.

So it works now?

@markus2330 In modern browsers it is not possible to display custom content in the beforeunload popup anymore. Should we use the default popup?

If it is possible to only show it if there are actual changes (like with pressing abort), yes please.