Fix-5698/show featured campaigns

[ujjwalpathaak]

What this PR does

Fixes #5698

Tasks

• [x] Basic functionality
• [x] Give setting page accessibility to all admins
• [x] Minor UI tweaks
• [x] Cover edge cases

  Demo Video & Database SS

  

Screencast from 19-03-24 11:37:57 PM IST.webm

[ujjwalpathaak]

@ragesoss I had a few doubts - 1) It was mentioned in the PR that rework the Settings page so that it is accessible to all admins, not just super-admins -- I think the settings page is already accessible to all the admins and not just super-admins

2) It also mentiones - with the existing features limited to super-admins but this new one also accessible to regular admins. -- Umm this is the code that is adding auth checks to settings controllers.

class SettingsController < ApplicationController # rubocop:disable Metrics/ClassLength
  before_action :require_admin_permissions
  before_action :require_super_admin_permissions,
                only: [:upgrade_admin, :downgrade_admin,
                       :upgrade_special_user, :downgrade_special_user,
                       :update_salesforce_credentials, :update_impact_stats,
                       :update_site_notice]

I think that not including the new controllers in only:require_super_admin automatically makes them accessible to both type of admins but all the other funcs not accessible to super_admins.

Is there anything I am understanding incorrectly or both the requirements are met?

---

3) Umm what all extra info would be needed for featured_campaigns_list in /settings

[ragesoss]

You're right, the settings page is already available to non-super admins. I guess I've had this thought before! This feature should be one that works for all admins.

I think the campaign slugs and titles as in your screenshot is fine. It would be nice to be able to control the order as well, although not necessary.

[ujjwalpathaak]

Hey, I was trying to find a way to be able to control the order of the campaigns. I thought of adding another attribute - "position" with the slugs

We could sort out the slugs based on these positions. To re-arrange these slugs we could just change the value of positions for which we could use drag-n-drop. But, react-dnd is the library that is used to handle this ( timeline.jsx ) but for that I will need to add this drag-n-drop functionality to /common/list.jsx and make it conditionally render this functioanlity for this particular use case based on a boolean prop.

Should I go ahead and try to add this library to List.jsx? or is adding this feature not that necessary, as you said.

[ragesoss]

@ujjwalpathaak i suggest that you stick with the current implementation and then you or I can open a new issue about adding drag-n-drop ordering. For the storage side of it, an explicit position shouldn't be necessary, as a serialized array is already ordered.

[ujjwalpathaak]

Sure, I will raise an issue for the same. Could you have a look and lemme know the changes required for this PR.

[ragesoss]

I will try it out soon. It should have some tests.

[ujjwalpathaak]

Hey, I tried writing a few tests for the explore page interaction. LMKWYT

1. How should the explore page react if there are featured campaigns listed?
2. How should the explore page react if NO featured campaigns are listed?

Also, Umm should I add tests for the new controllers too, they can be -

  describe '#add_featured_campaign' do
    it 'is campaign added successfully' do
    end
  end

  describe '#remove_featured_campaign' do
    it 'is campaign removed successfully' do
    end
  end

check for - if only admins are allowed to add campaigns isn't necessary as non-admins dont have access to the settings page.

[ragesoss]

No test needed for the admins vs non-admins part, but I suggest this for a set of tests:

Before: create a number of campaigns (more than 10) with different created-at dates.

1. Visit the explore page without doing anything to set featured campaigns, expect to see the latest campaigns by default.
2. Visit the settings page and add two featured campaigns that are not among the 10 most recent, then visit the explore page, expect to see the featured campaigns.

If you have those tests, I don't think any controller tests are necessary, but if you think there are things that are going to be in risk of getting broken by future changes in the controllers, you're welcome to add tests for them.

[ragesoss]

I tried this out and it works smoothly. One thing I noticed is that the header on the Explore page is "Newest Campaigns". I guess it should switch to "Featured Campaigns" when that's what is being shown.

[ujjwalpathaak]

I thought about this when I initially started to work on this PR but it slipped out of my mind.

I'll make the change + the required tests in the next commit.

[ujjwalpathaak]

Hey, I have refactored the tests and added dynamic header on explore page.

[ujjwalpathaak]

Hey, I need some help regarding the new test I wrote. In the first case where Newest Campaigns are expected I am not able to get the latest 10. ( I have removed the extra content ) The campaigns in the test are made in chronological order

Failures:

  1) explore page featured campaigns are NOT listed visit explore page
     Failure/Error: expect(page).not_to have_content(campaign1.title)
       expected not to find text "Test Campaign1" in "Explore\nMy Dashboard\nAdmin\nTraining\nen\nRagesauce\nLog 
Newest Campaigns
Test Campaign8
Test Campaign9
Test campaign20
Test campaign21
Test campaign22
Test Campaign1
Test Campaign2
Test Campaign3
Test Campaign4
Test Campaign5

Can you suggest me where should I look? As the test is working locally for me plus when I test this manually in my browser I am getting expected results.

[ragesoss]

The default ordering is based on created_at timestamp, and it may be happening fast enough that they all have the same timestamp. I suggest adding explicit created_at values to the setup.