New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix-5698/show featured campaigns #5714
Fix-5698/show featured campaigns #5714
Conversation
@ragesoss I had a few doubts -
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? |
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 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. |
Sure, I will raise an issue for the same. Could you have a look and lemme know the changes required for this PR. |
I will try it out soon. It should have some tests. |
Hey, I tried writing a few tests for the explore page interaction. LMKWYT
Also, Umm should I add tests for the new controllers too, they can be -
check for - if only admins are allowed to add campaigns isn't necessary as non-admins dont have access to the settings page. |
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.
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. |
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. |
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. |
Hey, I have refactored the tests and added dynamic header on explore page. |
Hey, I need some help regarding the new test I wrote.
Can you suggest me where should I look? |
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. |
What this PR does
Fixes #5698
Tasks
Demo Video & Database SS
Screencast.from.19-03-24.11.37.57.PM.IST.webm