Extend make visible in stats #2168

uzorjchibuzor · 2022-10-11T22:10:55Z

Closes #2142

Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!

…n Moments

…wn and therefore cannot be selected

…t is not visible, included translations

julianguyen

Thanks for taking this on! 🎉 Looking good so far!

julianguyen · 2022-10-12T04:50:46Z

client/app/components/Story/StoryActions.jsx

@@ -10,6 +10,7 @@ import {
  faDoorOpen,
  faDoorClosed,
  faExclamationTriangle,
+  faEyeSlash,


You'll want to update this part of the file to use this new icon.

I was able to use the icon eventually. I repeated it everywhere the tooltip is displayed. What I did is to pass the visibility status as a prop key. I will post the diff here for reference.

Here is where the conditional rendering is effected.

… where applicable

uzorjchibuzor · 2022-10-12T15:44:21Z

Is there a reason why the tests that are failing are not even remotely related to the things I did?

LMulvey · 2022-10-12T18:44:10Z

Is there a reason why the tests that are failing are not even remotely related to the things I did?

@uzorjchibuzor You likely have conflicting local settings for things like ESLint and Prettier–I know I did. Navigate to ifme/client and run yarn lint locally and then commit those changes–that will sync everything up with what CI expects! 💪🏻

uzorjchibuzor · 2022-10-12T19:39:29Z

Is there a reason why the tests that are failing are not even remotely related to the things I did?

@uzorjchibuzor You likely have conflicting local settings for things like ESLint and Prettier–I know I did. Navigate to ifme/client and run yarn lint locally and then commit those changes–that will sync everything up with what CI expects! 💪🏻

I wanted to install a VSCode extension before I tried your suggestion. Hope it works. My Linting is horrible.

uzorjchibuzor · 2022-10-12T19:51:06Z

I think I passed ESLint, CircleCI is Capybara, Let me know if I need to fix anything.

…es not respond to the :viewers method

julianguyen

Thanks so much for taking this on 🎉 Awesome work 🙌

app/helpers/categories_helper.rb

app/helpers/moments_helper.rb

app/helpers/viewers_helper.rb

… methods

sible, not_visible) resolves to false, it will be removed from the returned hash so as not to cause a Type error in the StoryAction.jsx

julianguyen

This is coming along really well 🙌 🎉 Thanks for all the updates!

There's one edge case that we should address!

julianguyen · 2022-10-15T08:09:41Z

app/controllers/moments_controller.rb

@@ -127,6 +127,6 @@ def associated_strategies
    Strategy.where(user: @viewers).each do |strategy|
      strategy_ids << strategy.id if strategy.viewer?(current_user)
    end
-    Strategy.where(id: strategy_ids).order(created_at: :desc)
+    Strategy.is_visible.where(id: strategy_ids).order(created_at: :desc)


An edge case that we should account for is what happens when we create a Moment with a Strategy/Category/Mood OR a Strategy with a Category that is visible when created, but is later edited to be not visible?

Example:

Create a Moment with a Strategy that is visible

Edit the Strategy and make it not visible

View the Moment and the non-visible Strategy is still listed (this is fine!)

Edit the Moment and the Strategy is not in the list (this is the issue!)

I think the expected behaviour for 4) should be that we can view the selected Strategy. If we uncheck the box for it and save our changes, then we can no longer add it back.

Do you mean that if an element is tagged, it should remain visible to the parent?

I think if it's already tagged, but the tag is made not visible, it should still be visible to the post (or parent like you said) that it's being used in.

I think I understand now. It's late here but I should have a commit tomorrow, I will have to do away with the scope Concern though.

julianguyen

Thanks for the updates and adding tests along the way! This is coming along great 🎉

julianguyen · 2022-10-19T09:34:13Z

spec/requests/strategies_spec.rb

@@ -142,6 +151,18 @@
        end
      end

+      it 'reders associated category in dropdown regardless of visibility status' do


Grammar: reders should be spelled as renders

julianguyen · 2022-10-19T09:46:09Z

app/controllers/moments_controller.rb

@@ -109,9 +109,9 @@ def moment_params
  end

  def set_association_variables!
-    @categories = current_user.categories.order(created_at: :desc)
+    @categories = current_user.categories.is_visible.or(Category.where(id: @moment.category_ids)).order(created_at: :desc)


I found a bug:

Create a Moment with a visible Category

Edit the Category to be not visible

Edit the Moment and uncheck the Category and hit "Submit"

The Category is not removed (it should be removed)

I think I understand but I want to recreate this to be sure,

The issue is that if we're editing a Moment or Strategy, we're overriding tags that are removed before they are saved.

julianguyen · 2022-10-19T09:46:19Z

app/controllers/moments_controller.rb

    @category = Category.new
-    @moods = current_user.moods.order(created_at: :desc)
+    @moods = current_user.moods.is_visible.or(Mood.where(id: @moment.mood_ids)).order(created_at: :desc)


Similar bug as above, but for Moods!

julianguyen · 2022-10-19T09:46:45Z

app/controllers/moments_controller.rb

@@ -127,6 +127,6 @@ def associated_strategies
    Strategy.where(user: @viewers).each do |strategy|
      strategy_ids << strategy.id if strategy.viewer?(current_user)
    end
-    Strategy.where(id: strategy_ids).order(created_at: :desc)
+    Strategy.is_visible.where(id: strategy_ids).or(Strategy.where(id: @moment.strategy_ids)).order(created_at: :desc)


Similar bug as above, but for Strategies!

julianguyen · 2022-10-19T09:47:34Z

app/controllers/strategies_controller.rb

@@ -52,7 +52,7 @@ def edit
      redirect_to_path(strategy_path(@strategy))
    end
    @viewers = current_user.allies_by_status(:accepted)
-    @categories = current_user.categories.order('created_at DESC')
+    @categories = current_user.categories.is_visible.or(Category.where(id: @strategy.category_ids)).order('created_at DESC')


I found a bug:

Create a Strategy with a visible Category

Edit the Category to be not visible

Edit the Strategy and uncheck the Category and hit "Submit"

The Category is not removed (it should be removed)

…s passed un update

uzorjchibuzor

the commit message is supposed to say Moment and Strategy is now able to remove associations when no params is passed to the controller by a before_save callback

…s is passed to the controller by a before_save callback

julianguyen

Fantastic work!! Thanks for all the updates as well 🎉

uzorjchibuzor added 6 commits October 11, 2022 17:13

used scope to ensure that Moods set to not_visible are not tag-able i…

bf1b18f

…n Moments

tests for mood visibility in moments

24b4cdd

categories and strategy set to visible: false are not shown in dropdo…

4634967

…wn and therefore cannot be selected

changed label with translations

c29d014

showing "Not visible to tag and display in profile stats" when elemen…

6aaeab0

…t is not visible, included translations

reflect changes to visible_helper in spec

2412ba7

uzorjchibuzor added the wip label Oct 11, 2022

rendering icons based on mood visibility

6b7c09d

julianguyen reviewed Oct 12, 2022

View reviewed changes

rendered icons conditionally according according to visibility status…

df3fed5

… where applicable

uzorjchibuzor removed the wip label Oct 12, 2022

fix rubocop offenses

58a2629

yarn lint run to solve circle cI --fix

dfb3d96

julianguyen requested a review from rohandaxini October 12, 2022 21:06

uzorjchibuzor added 4 commits October 13, 2022 14:26

corrected failure test on .storyActionsViewers visiblity problem

d2cb633

fixed conflict with main branch

2adf3a2

added conditional to viewers helper to account for when an element do…

46a0bc2

…es not respond to the :viewers method

add skipped translation to de.yml

8ebd255

julianguyen requested changes Oct 14, 2022

View reviewed changes

app/helpers/categories_helper.rb Outdated Show resolved Hide resolved

app/helpers/moments_helper.rb Outdated Show resolved Hide resolved

app/helpers/viewers_helper.rb Show resolved Hide resolved

uzorjchibuzor added 2 commits October 14, 2022 09:47

applied suggested changes to DRY the changes made to visibility based…

8aecca5

… methods

when a visibility key (vi

8e8e20c

sible, not_visible) resolves to false, it will be removed from the returned hash so as not to cause a Type error in the StoryAction.jsx

uzorjchibuzor requested a review from julianguyen October 14, 2022 09:52

julianguyen requested changes Oct 15, 2022

View reviewed changes

uzorjchibuzor added 3 commits October 17, 2022 07:49

associated elements are displayed on edit regardless of visiblity status

2761205

test to ensure edited elements retain association on edit

6947872

fix merge conflict

049eae6

uzorjchibuzor requested a review from julianguyen October 17, 2022 07:26

julianguyen requested changes Oct 19, 2022

View reviewed changes

uzorjchibuzor added 3 commits October 20, 2022 10:19

rixed typo `reders => renders

10ef1f8

Moment and Strategy is unable to remove associations when no params i…

1face49

…s passed un update

removed redundant model methods and accompanying tests

29e3459

uzorjchibuzor commented Oct 20, 2022

View reviewed changes

Moment and Strategy are now able to remove associations when no param…

d972dc6

…s is passed to the controller by a before_save callback

uzorjchibuzor requested a review from julianguyen October 20, 2022 13:25

uzorjchibuzor added 2 commits October 20, 2022 14:41

update nokogiri to 1.13.9 pass circle ci

9090ff4

removed left over comments from code

29996ff

julianguyen approved these changes Oct 20, 2022

View reviewed changes

julianguyen merged commit a3e05a8 into main Oct 20, 2022

delete-merged-branch bot deleted the extend_make_visible_in_stats branch October 20, 2022 23:56

julianguyen added the hacktoberfest-accepted label Oct 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend make visible in stats #2168

Extend make visible in stats #2168

uzorjchibuzor commented Oct 11, 2022

julianguyen left a comment

julianguyen Oct 12, 2022

uzorjchibuzor Oct 12, 2022 •

edited

Loading

uzorjchibuzor Oct 12, 2022

uzorjchibuzor commented Oct 12, 2022

LMulvey commented Oct 12, 2022

uzorjchibuzor commented Oct 12, 2022

uzorjchibuzor commented Oct 12, 2022

julianguyen left a comment

julianguyen left a comment

julianguyen Oct 15, 2022 •

edited

Loading

uzorjchibuzor Oct 15, 2022 •

edited

Loading

julianguyen Oct 15, 2022

uzorjchibuzor Oct 15, 2022 •

edited

Loading

julianguyen left a comment

julianguyen Oct 19, 2022

julianguyen Oct 19, 2022

uzorjchibuzor Oct 19, 2022

julianguyen Oct 19, 2022

julianguyen Oct 19, 2022

julianguyen Oct 19, 2022

julianguyen Oct 19, 2022

uzorjchibuzor left a comment

julianguyen left a comment

Extend make visible in stats #2168

Extend make visible in stats #2168

Conversation

uzorjchibuzor commented Oct 11, 2022

julianguyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uzorjchibuzor Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uzorjchibuzor commented Oct 12, 2022

LMulvey commented Oct 12, 2022

uzorjchibuzor commented Oct 12, 2022

uzorjchibuzor commented Oct 12, 2022

julianguyen left a comment

Choose a reason for hiding this comment

julianguyen left a comment

Choose a reason for hiding this comment

julianguyen Oct 15, 2022 • edited Loading

Choose a reason for hiding this comment

uzorjchibuzor Oct 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uzorjchibuzor Oct 15, 2022 • edited Loading

Choose a reason for hiding this comment

julianguyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uzorjchibuzor left a comment

Choose a reason for hiding this comment

julianguyen left a comment

Choose a reason for hiding this comment

uzorjchibuzor Oct 12, 2022 •

edited

Loading

julianguyen Oct 15, 2022 •

edited

Loading

uzorjchibuzor Oct 15, 2022 •

edited

Loading

uzorjchibuzor Oct 15, 2022 •

edited

Loading