Added option: Hide step ingredients

[srwareham]

This pull request addresses #2266 by adding a new cosmetic setting. When the setting is left in its default state, the UI renders exactly as it does currently. When the "Show Step Ingredients" setting is toggled to off, the steps of a recipe will not include an ingredients table.

Screenshots:

1. Defaults to status quo behavior
   
2. UI with default status quo
   
3. UI when new setting is enabled
   

[vabene1111]

Hi, thank you for your well documented PR.

Adding this setting has been in discussion for a long time and there are a few github issues talking about pros and cons. I need to think about if I want to merge this as I was leaning more towards a recipe based setting instead of a general setting.

Will leave this open for now and consider which way I want to go

[srwareham]

OK, thank you. Would it make sense to consolidate the discussions in one place?

[stleusc]

how about a global setting with a recipe override?

[stleusc]

Or even better...
The ingredients could be turned off per step. Simply a 'hide ingredients'.

That would allow for one to create a step 1 with ONLY ingredients, no text and then hide the ingredients.

[vabene1111]

I think I would like a per step setting with a sensible default (the way it currently is). There area also currently some automatic rendering rules (e.g. a recipe with one step never renders ingredients on the step) which will need to be taken into account.

[srwareham]

Renamed the underlying branch, which accidentally closed this PR. I was looking into this but think there will have to be UI decisions about how to show this before it would make sense trying to wire up the backend DB changes.

I think some of this PR could be reused to set default user level settings. Which could then be overwritten on a step-by-step basis.

@vabene1111 I don't really have frontend dev experience. What do you think is a reasonable way to give this per step control?

[vabene1111]

Thanks. The view is probably the easy part, just check for a book property added to the step model similar to the show title.

The editor is a bit trick, I already dislike the three dot menu but to be honest we can add it there for now, will need to change that in the future. So basically you can orientate yourself by the show title property (not sure exactly what it's called)

[srwareham]

So something like changing the new default UI to have a new element like this for each step

And then, if someone has clicked hide ingredients table, it would change to something like this:

What do you think? I'd also keep the user level setting from this PR and just flip the default behavior in these screeenshots if the alternate user preference has been set

[vabene1111]

yes, that sounds good, just need to communicate on the settings page then that its "the default", thanks!

[srwareham]

I've updated the PR to save step-level state whether the ingredients table should be shown or not. I also still have the same user-level preference setting, but haven't figured out how to incorporate that into the step-level design. (will rename and such after have working fully).

@vabene1111 would you have any guidance how to set the Step model's default show_ingredients state to respect the currently signed in user's preference? I couldn't figure out how to access the current user's UserPreference from within model.py. From what I could find, it seemed like overwriting the save method for the Step model might be how to do this? I tried using examples online but didn't succeed.

At a high level, I don't have a good understanding of the data model for how a user (and their preferences) should interact with a recipe step. From the comments earlier in this PR, the guidance was that the user preference should be reflected on a per step basis. Are recipe step's always 1 step to 1 user that can ever see them? Or might multiple users see the same step of a recipe? Should they all see the same configuration, or can they have different preferences and therefore the same state should be recorded on a per user-step basis? For simplicity's sake, it may make more sense to respect the preferences used at time of creation/modification.

Thanks!

[vabene1111]

hi, thanks for the great work and comment

1. there are several places you could use the global preference, each have their advantages and disadvantages. Without explaining them all I would like to go the way we go with most of these features for consistency reasons (at least for now): implement them on the frontend. You can access a user preference on the frontend using the useUserPreferenceStore (see MealPLanEditModal.vue for reference how to use it). Then, when adding a new step in the frontend, just set the "hidden" property to whatever the user preference is
2. Multiple people can see the same step with different default properties BUT in my opinion the author of a recipe "designs" it. if the author/last editor decides that ingredients should be hidden than they should be. Thus I suggest we only use the setting when creating new steps in a recipe

[srwareham]

Thank you! Do you have thoughts how that would interact with the URL import mechanism for recipes? I'd like to have an imported recipe respect the importing user's preferences without needing to manually re-open and edit it. Is there a way to use the frontend paradigm in that workflow?

[stleusc]

@srwareham I know, it's off-topic but I am wondering, how much work it would be to get another 'setting' on each step to force-show the notes in the browser.

PS: This PR is one of the most needed things for me! Thank you so much for your work!

[vabene1111]

Do you have thoughts how that would interact with the URL import mechanism for recipes?

interesting 🤔 I guess the most "intuitive" thing would be to set the step / all steps to the user preference when importing. I dont really want to build a "second" recipe editor for the import so dont want to add any options there.

This can/must be done in two places, firstly on the url import part in the backend and secondly when creating new steps in the pre-import editor. If you need any help finding them let me know.

Do you agree that this would be what users would expect to happen?

[srwareham]

I'll poke around, I think I know where those are. And, yep, I believe this will well meet user expectations. I'll also rework the language in the help text of the setting once I've got the implementation working.

[srwareham]

I figured there'd be some intutive changes to make in ImportView.vue or ImportViewStepEditor.vue, but it seems as though they're strictly working with JSON objects. I assumed it'd be easy enough to modify the internal steps if they were Recipe objects. Would you be able to point me where you think the changes should be made?

The only other places I came across that might have made sense were in cookbook/views/new.py or cookbook/views/, but I don't think that's what you have in mind

[vabene1111]

I figured there'd be some intutive changes to make in ImportView.vue or ImportViewStepEditor.vue, but it seems as though they're strictly working with JSON objects. I assumed it'd be easy enough to modify the internal steps if they were Recipe objects. Would you be able to point me where you think the changes should be made?

this is indeed a bit more hidden that I had remembered it. I think this https://github.com/TandoorRecipes/recipes/blob/develop/vue/src/apps/ImportView/ImportViewStepEditor.vue#L141 would be the best place, here a new step object is created which would lack the property hidden true/flase

the other location relevant for the import would be here https://github.com/TandoorRecipes/recipes/blob/develop/cookbook/helper/recipe_url_import.py#L150

technically the setting could also be respected for all "app" imports but i think this is something that can be added later down the line as well (although it could be very useful ...)

[srwareham]

Updated PR to support user-level default settings. Also cleaned up the setting name and help text. This should be fully functional now. I have a comment above about an edge case I'd like to improve. That said, this should be ready for code review/testing for everything else.

After review and prior to merge, would you like commits squashed or left as is?

Thanks!

[vabene1111]

that sounds great, i have added a comment to your question that should resolve your issue.

When thats done i guess squashing is a good idea and also please remove the migration, I will re-run makemigrations after merging, thats just a little bit easier than merging them manually.

One last thing we should discuss: Do we want to implement this setting in the importers as well? If so do you want to do it or should I after merge?

[srwareham]

@vabene1111 Thank you for the pointer! I've fixed the implementation to not edit addIngredient. I've also modified all of the integrations to pull in the user-level default and updated the settings help string accordingly. I have only tested the URL and manually created recipes as I don't have any of the other services. Lastly, I've removed the migrations and squashed the commits.

So, in summary, the feature implemented is:

1. The ability to set a step-level setting whether or not the ingredients table should be shown.
2. A user-level default setting that is respected when importing or creating a recipe. This applies at recipe creation time and can be overridden in the edit menu at any time

I haven't applied any style linters. Should be ready to merge and lint as you prefer. I appreciate the active help!

[srwareham]

@vabene1111 anything I can do to help merge?

[vabene1111]

I am sorry for not getting back to this earlier, its been some stressfull weeks for me.

I wanted to merge this but came across one question that I was really unsure and want your feedback on, see the review comments above. once thats done I will get this merged as soon as i can.

[srwareham]

Life happens, no worries. Thank you for even taking the time.

I clicked resolve on the only comment I could find--I had already gotten rid of the hacky workaround from the PR. Is there something else you'd like me to look through before merge?

[vabene1111]

oh i forgot to send the review again, i always feel aftert addingn the comments that they are already visible but i need to click one more time. Now they should be visible for you as well.

[srwareham]

Thank you! I have resolved the comments by removing the lines in question. I've squashed the commits in with the PR and should be merge-friendly

[vabene1111]

perfect, thank you. Got it merged for the next release.