Migrate Create newcomers entry page to react #10732

danieljames-dj · 2025-01-28T14:50:51Z

This PR aims at converting the create-newcomers page to react

gregorbg · 2025-01-28T15:08:44Z

app/webpacker/components/Panel/pages/CreateNewComersPage/index.jsx

You can definitely use SemUI components here.

Done, I assume
doesn't have any alternative and hence can be used.

gregorbg · 2025-01-28T15:09:49Z

app/webpacker/components/Panel/PanelPages.jsx

@@ -169,7 +169,7 @@ export default {
  },
  [PANEL_PAGES.createNewComers]: {
    name: 'Create New Comers',
-    link: createNewComersUrl,
+    component: CreateNewComersPage,


Not 100% sure but I think the correct capitalization is just "Newcomers".

Oh yes, changed. (there are some places where it's still wrong, I'll change that in a separate PR after this is merged, because those are not related to migration)

gregorbg · 2025-01-28T15:11:09Z

app/controllers/admin_controller.rb

-      competition_ids: params[:competition_ids] || nil,
-    )
+  private def competition_list_from_string(competition_ids_string)
+    (competition_ids_string || '').split(',').uniq.compact


You can make sure that there's always a valid string being passed to this method via stuff like params.fetch(:foo, 'my_default')

gregorbg · 2025-01-28T15:12:13Z

app/webpacker/components/Panel/pages/CreateNewComersPage/CompetitionsInput.jsx

+      <Header as="h4">
+        <HeaderSubheader>
+          Leave blank to check for all competitions
+        </HeaderSubheader>
+      </Header>


Creating a header only for showing a subheader (with no "main" header) smells like bad code to me. What is your goal here, and why is a simple text paragraph not good enough?

I wanted to show it as 'hint' like thing, so I followed your suggestion in another PR (LINK). Did I implement this wrongly?

maxidragon · 2025-01-28T15:36:55Z

app/webpacker/components/Panel/pages/CreateNewComersPage/index.jsx

+export default function CreateNewComersPage() {
+  return (
+    <>
+      <Header>Create New Comers</Header>


Suggested change

<Header>Create New Comers</Header>

<Header>Create newcomers</Header>

maxidragon · 2025-01-28T17:49:00Z

app/webpacker/components/Panel/PanelPages.jsx

@@ -169,7 +169,7 @@ export default {
  },
  [PANEL_PAGES.createNewComers]: {
    name: 'Create New Comers',


Suggested change

name: 'Create New Comers',

name: 'Create newcomers',

This should be changed as well ;-)

Okay now since you said it, I've changed it :-)

As mentioned in #10732 (comment), I wanted to change them separately all together in another PR after this is merged. For example, the line above the line you highlighted - for that to be changed, I need to change at many places which will make this PR more confusing and those changes will not be aligned to the PR title.

gregorbg · 2025-02-03T10:14:18Z

app/views/admin/_import_inbox_step.erb

@@ -49,7 +49,7 @@
        All newcomers seem to have been assigned WCA IDs.
        Please <%= link_to "delete the inbox rows", competition_admin_delete_inbox_data_path(@competition, model: :inbox_person), class: 'inbox-trigger btn btn-primary', data: { jquery_method: :delete } %>.
      <% else %>
-        Please <%= link_to "assign WCA IDs to newcomers", admin_complete_persons_path(finish_persons_form: { competition_ids: @competition.id }), target: '_blank', class: 'btn btn-primary' %>
+        Please <%= link_to "assign WCA IDs to newcomers", admin_complete_persons_path(competition_ids: [@competition.id]), target: '_blank', class: 'btn btn-primary' %>


Why do you need to wrap this in an array? Passing it as a string should be just fine

Yes, it do accept single string as well I guess, but there are cases where multiple competitions are also needed to be given as input. So, to make the type of competition_ids as a single type (array of string) instead of multiple type (array of string or single string), I thought of doing this way. Is it fine?

Well, right here you're passing it to a *_path Rails helper, which encodes it into a URL anyways. So when reading it again in the controller, won't it end up as a string in each case?

Sorry my previous comment wasn't specific, but what I'm surprised about is the fact that you explicitly wrap it in an array just before encoding it into a string URL.

What is the difference in terms of the resulting URL anyways?

Oh my bad, you are right. I've changed it now, thank you.

gregorbg · 2025-02-03T10:15:02Z

app/views/admin/complete_persons.html.erb

@@ -1,18 +1,18 @@
 <% provide(:title, 'Finish unfinished persons') %>

 <div class="container">
-  <% competition_ids = @finish_persons.competition_ids %>
+  <% competition_ids = @competition_ids %>


This line is redundant now, if you already have a @competition_ids variable then use it everywhere.

No need to re-assign it to "itself" with another name

Okay, instead of changing in many places, I thought of doing this way. Changed all the occurrences.

gregorbg · 2025-02-03T10:15:45Z

app/views/admin/complete_persons.html.erb


  <h2>
    <%= yield(:title) %>
-    <%= link_to "Go back", admin_finish_unfinished_persons_path(competition_ids: competition_ids), class: "btn btn-default" %>
+    <%= link_to "Go back", panel_page_path(id: User.panel_pages[:createNewComers], competition_ids: competition_ids), class: "btn btn-default" %>


Does this work as expected when competition_ids is an already-split array?

Thanks for pointing that out, I actually didn't test that portion and realized that it was not even working for single competition ID as it was getting lost during redirection. I've fixed it now.

gregorbg · 2025-02-05T10:20:26Z

app/views/admin/_import_inbox_step.erb

@@ -49,7 +49,7 @@
        All newcomers seem to have been assigned WCA IDs.
        Please <%= link_to "delete the inbox rows", competition_admin_delete_inbox_data_path(@competition, model: :inbox_person), class: 'inbox-trigger btn btn-primary', data: { jquery_method: :delete } %>.
      <% else %>
-        Please <%= link_to "assign WCA IDs to newcomers", admin_complete_persons_path(finish_persons_form: { competition_ids: @competition.id }), target: '_blank', class: 'btn btn-primary' %>
+        Please <%= link_to "assign WCA IDs to newcomers", admin_complete_persons_path(competition_ids: [@competition.id]), target: '_blank', class: 'btn btn-primary' %>


Well, right here you're passing it to a *_path Rails helper, which encodes it into a URL anyways. So when reading it again in the controller, won't it end up as a string in each case?

Sorry my previous comment wasn't specific, but what I'm surprised about is the fact that you explicitly wrap it in an array just before encoding it into a string URL.

What is the difference in terms of the resulting URL anyways?

gregorbg · 2025-02-05T10:21:25Z

app/views/admin/complete_persons.html.erb

  <h2>
    <%= yield(:title) %>
-    <%= link_to "Go back", admin_finish_unfinished_persons_path(competition_ids: competition_ids), class: "btn btn-default" %>
+    <%= link_to "Go back", panel_page_path(id: User.panel_pages[:createNewComers], competition_ids: @competition_ids.join(',')), class: "btn btn-default" %>


In regards to my other comment above: Kindly pointing out that here, you're fine with comma-glueing, but above in the other URL helper you insist on array-wrapping 😉

Oops ya 😅 my bad.

gregorbg · 2025-02-05T10:22:31Z

app/views/admin/complete_persons.html.erb

        <li><code><%= competition_id %></code></li>
      <% end %>
    </ul>
  <% end %>

  <%= simple_form_for :person_completions, url: admin_complete_persons_path do |f| %>
-    <%= f.hidden_field :competition_ids, value: competition_ids %>
+    <%= f.hidden_field :competition_ids, value: @competition_ids %>


Again, this creates an input field in the DOM tree. How is this being handled if @competition_ids is an array?

(In other words: Should you manually glue them together with join(',') or is that not necessary?

Thanks for the catch. I fixed it now.

I tested with one competition and no competition. When I input no competition, it will run this script for all competitions, and I wrongly assumed that multiple competitions case will also be covered there.

gregorbg · 2025-02-05T10:24:19Z

app/webpacker/components/Panel/pages/CreateNewcomersPage/CompetitionsInput.jsx

+      <Button
+        primary
+        size="big"
+        href={viewUrls.admin.completePersons(competitionIds.length > 0 ? competitionIds : null)}


Why is your jsonToQueryString in the JS route definition not clever enough to catch the case when competitionIds is an empty array?

(and can you adjust it easily to make it clever?)

I just realized that jsonToQueryString was able to handle empty array, so I removed the check now. :-)

github-actions bot assigned danieljames-dj Jan 28, 2025

gregorbg requested changes Jan 28, 2025

View reviewed changes

maxidragon reviewed Jan 28, 2025

View reviewed changes

danieljames-dj force-pushed the create-newcomers1 branch 4 times, most recently from 5259a4c to b13e2ce Compare January 28, 2025 16:45

maxidragon reviewed Jan 28, 2025

View reviewed changes

danieljames-dj force-pushed the create-newcomers1 branch from b13e2ce to 0c0c0cf Compare January 29, 2025 03:40

gregorbg requested changes Feb 3, 2025

View reviewed changes

danieljames-dj force-pushed the create-newcomers1 branch from d3cfb9b to 5b3a3a5 Compare February 3, 2025 12:40

gregorbg requested changes Feb 5, 2025

View reviewed changes

danieljames-dj added 5 commits February 5, 2025 22:07

Migrate Create newcomers entry page to react

7cace9d

Removed 2 unused routes

e127bbb

Review changes

a5158ab

Review changes

469ee34

Review changes

4cdd502

danieljames-dj force-pushed the create-newcomers1 branch from 5b3a3a5 to 4cdd502 Compare February 6, 2025 05:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Create newcomers entry page to react #10732

Migrate Create newcomers entry page to react #10732

danieljames-dj commented Jan 28, 2025

gregorbg Jan 28, 2025

danieljames-dj Jan 28, 2025

gregorbg Jan 28, 2025

danieljames-dj Jan 28, 2025

gregorbg Jan 28, 2025

danieljames-dj Jan 28, 2025

gregorbg Jan 28, 2025

danieljames-dj Jan 28, 2025

maxidragon Jan 28, 2025

danieljames-dj Jan 28, 2025

maxidragon Jan 28, 2025

danieljames-dj Jan 29, 2025

gregorbg Feb 3, 2025

danieljames-dj Feb 3, 2025

gregorbg Feb 5, 2025

danieljames-dj Feb 6, 2025

gregorbg Feb 3, 2025

gregorbg Feb 3, 2025

danieljames-dj Feb 3, 2025

gregorbg Feb 3, 2025

danieljames-dj Feb 3, 2025

gregorbg Feb 5, 2025

gregorbg Feb 5, 2025

danieljames-dj Feb 6, 2025

gregorbg Feb 5, 2025

gregorbg Feb 5, 2025

danieljames-dj Feb 6, 2025

gregorbg Feb 5, 2025

gregorbg Feb 5, 2025

danieljames-dj Feb 6, 2025

	<Header>Create New Comers</Header>
	<Header>Create newcomers</Header>

Migrate Create newcomers entry page to react #10732

Are you sure you want to change the base?

Migrate Create newcomers entry page to react #10732

Conversation

danieljames-dj commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment