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

Refactor: Use current_conference instead of set_conference and conference (1/3) #2303

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

onk
Copy link
Collaborator

@onk onk commented May 5, 2024

  1. Unify Access to @conference
    • Avoid using return value of set_conference:
      • Call set_conference and use @conference ivar; instead of using the return value from set_conference.
    • Ensure set_conference is always called via before_action:
      • Ensure that @conference is always set before any action that using @conference is executed.
    • Deprecate conference Method:
      • Remove usage of the redundant conference method. this can replaced with using @conference ivar.set by set_conference.
  2. Transition from @conference to current_conference method
    • Implement current_conference:
      • Introduce a new method current_conference that will centralize the access to the conference data.
    • Replace all @conference with current_conference
  3. Cleanup
    • Remove conference method:
      • After confirming its redundancy, the conference method will be removed from the code base.
    • Remove unnecessary before_action:
      • Since current_conference reliably setting conference data, set_conference can be removed from before_action hooks.

Motivation

The application currently has four different ways to access the conference data, leading to confusion and inconsistent usage:

  • set_conference method:
    • Defined in ApplicationController, sets and returns @conference.
    • Called inconsistently as a before_action or not, and its return value is used.
  • conference method:
    • Exists in both Secured and SecuredAdmin concern modules with the same implementation as set_conference.
    • This is not used in before_action and is called when its return value is needed.
  • @conference ivar:
    • Set by either set_conference or conference method.
    • Its usage is unreliable as set_conference isn't always called.
  • Direct calls in actions:
    • Uses Conference.find_by(abbr: params[...]) in actions where neither before_action nor instance variable setups are in place.

This refactor aims to unify and simplify how the conference data is accessed within the application.

After completing these changes, we can use current_conference to access the conference data.

The chosen method name current_conference comes from the commonly used current_user and improves readability compared to conference and avoids confusion with local variables.

onk added 3 commits May 6, 2024 00:01
ApplicationController has `set_speaker` method.

```ruby
def set_speaker
  if current_user
    @speaker = Speaker.find_by(email: current_user[:info][:email], conference_id: set_conference.id)
  end
end
```

SpeakerDashboardsController's method is

```ruby
def set_speaker
  @conference ||= Conference.find_by(abbr: params[:event])
  if current_user
    @speaker = Speaker.find_by(conference_id: @conference.id, email: current_user[:info][:email])
    @talks = @speaker ? @speaker.talks.not_sponsor : []
  end
end
```

Only differences are related to setting @talks, so setting of `@talks` into the action.

Note: `set_conference` implementation is here.

https://github.com/cloudnativedaysjp/dreamkast/blob/6c4b9071bf895dea2f8006184382ed39e1ef885a/app/controllers/application_controller.rb#L128-L130
https://github.com/cloudnativedaysjp/dreamkast/blob/6c4b9071bf895dea2f8006184382ed39e1ef885a/app/controllers/application_controller.rb#L39-L41

```ruby
def set_conference
  @conference ||= Conference.find_by(abbr: event_name)
end

def event_name
  params[:event]
end
```
Use ivar `@conference` which is set by `set_conference` instead of `set_conference`'s return value.
…ncerns

`set_conference` is now invoked automatically via `before_action` in the `Secured` and `SecuredAdmin` concerns.
This change removes the need for explicit calls to `set_conference` in individual controller actions.
Copy link

github-actions bot commented May 5, 2024

@github-actions github-actions bot added the reviewapps Build ReviewApp environment automatically if this label is granted label May 5, 2024
onk added 4 commits May 6, 2024 02:13
Since `Secured` and `SecuredAdmin` modules already include the `set_conference` method as a `before_action`, the ivar `@conference` is consistently available.
Remove redundant method calls to `conference`.
Enforce the calling `set_conference` before `set_profile` across controllers to access `@conference` ivar.
`Secure` or `SecureAdmin` concerns already called `set_conference` by `before_action`.
For api/v1 routes, explicitly call `set_conference` in each controller.
Copy link

github-actions bot commented May 5, 2024

Simplecov Report

Covered Threshold
60.85% 60%

@github-actions github-actions bot removed the reviewapps Build ReviewApp environment automatically if this label is granted label Jul 23, 2024
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.

1 participant