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: upgrade organizer controllers to 1.7 #196

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Afonso-santos
Copy link
Contributor

@Afonso-santos Afonso-santos commented Sep 27, 2023

I upgrade organizer controllers to 1.7

@reviewpad
Copy link

reviewpad bot commented Sep 27, 2023

AI-Generated Summary: This pull request includes several enhancements to the organizer controller in the Bokken Web application.

Firstly, the organizer_controller.ex has been upgraded, with changes including the modification of controller usage, changes in rendering view semantics, and changes in response headers. For example, the controller now uses BokkenWeb, :controller instead of BokkenWeb, controller: "1.6". The render function now uses atom instead of string for view rendering. The location header has been manually written instead of using the Routes.organizer_path.

Additionally, a new file organizer_json.ex has been created to handle the data conversion of the organizer.

The organizer_view.ex has been deleted and it seems its functionalities have been moved to the new organizer_json.ex.

Finally, test cases for organizer_json.ex have been added to ensure the correct working of the functions provided by the module. These tests cover rendering a single organizer (show) and a list of organizers (index).

The updates made are aimed at improving the structure and readability of the code, enhance separation of concerns, and provide unit tests to validate the behavior of the organizer JSON handling.

Copy link
Member

@danielsp45 danielsp45 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to replace this:
image

@Afonso-santos
Copy link
Contributor Author

To upgrade to 1.7 we need to transform the views files to JSON files and delete the views. So we don't need to change the views files, right ?

@danielsp45
Copy link
Member

Merge with main 🙏🏻

Copy link
Member

@ruilopesm ruilopesm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should wait for #198 to be merged first, because it adds tests for a functionality that is being changed here.

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.

3 participants