Skip to content
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

Add Redirect For Requests For The Current Term Of Alerts #5805

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cyrillefr
Copy link
Contributor

What this PR does

This PR aims to close #5800

  • One can now bookmark:
    • /campaigns/current/alerts and/or
    • /campaigns/current/alerts.json

With a new route rule to go to the new current_alerts method in the Campaign controller.
Inspired by the alerts method from same controller.

The show method had to be modified because React calls some campaign/slug.json after populating with some
other calls. Whether in html or json format.
So the current has to be interpolated to the real current Campaign
Current is the same as default

Open questions and concerns

I believe one could add also this bookmark: http://domain/campaigns/current/programs quite easily

@ragesoss
Copy link
Member

What if we try to make a campaign with the slug current? I guess that should be prevented with a validation in campaign.rb, or some other way.

@cyrillefr
Copy link
Contributor Author

Hello @ragesoss ,

I need to check about the validation.
In case it is possible, this record should be updated with the values of the current campaign. Or may be some kind of link if it is possible.
I will investigate and in case post some code.

@cyrillefr
Copy link
Contributor Author

Hello @ragesoss ,

One can dup a Campaign object and give it the current slug (and also title). But such an object has no courses.
There is a need to do some other works to get the copy to link to associated objects of the current Campaign.

But also, how to keep these 2 objects in synch ?

We would need to add a hook on save/update on Campaign, check if it is about the current campaign, and if so, proceed to update current.slug Campaign. In my opinion, this is a bit heavy.

@ragesoss
Copy link
Member

@cyrillefr i was thinking that we should just use a validation on the Campaign model to make sure we never have an actual campaign with the slug current. Then way you've implemented it in the controller will work fine.

@cyrillefr
Copy link
Contributor Author

Aaaah :)
I added the validation.

@cyrillefr
Copy link
Contributor Author

Checks failed at linting stage. But for a source file version that is not part of my branch ....
File spec/controllers/articles_controller_spec.rb that have a line too long.
Version of my branch is older and does not have this modification.
A bit strange

@ragesoss
Copy link
Member

My fault. I added that linting violation while fixing an urgent but last week. Don't worry about that failure.

- One can now bookmark:
  - /campaigns/current/alerts and/or
  - /campaigns/current/alerts.json
- a new route rule to go to
- the new current_alerts method in the Campaing controller
- that is inspired by the alerts method
- show method modified bc React calls some campaign/slug.json
- whether in html or json format.
- slug when value current has to be interpolated right
- the corresponding spec
 - slug cannot be assigned name "current"
@cyrillefr cyrillefr force-pushed the AddRedirectForRequestsForTheCurrentTermOfAlerts branch from 64d4185 to 37064b2 Compare May 22, 2024 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add redirect for JSON and HTML requests for the current term of Alerts
2 participants