-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add a page with reindex functionality #101
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
Conversation
spaenleh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for you first contribution to the admin project 🎉
Overall it looks nice and the demo videos show that it also works !
I have a few comments on the code to align with the existing style.
Regarding tests, I see that you use application config to set the http_client and you build a fake one in the tests. I see multiple issues with this.
Application configuration is global this means that when you use Application.put_env in a test it changes for all the code that is currently executing. This means that parallel tests will have their configuration changed and this can lead to issues.
- For this I would propose to use something like the
Req.Testfrom the Req package. You can define stubs, they have nice example. This is concurrency safe.
I am unsure about how we need to handle the required env vars.
If we think that the application can absolutely not start without them, we should use System.fetch_env!/1 which will raise if the value is not defined and halt the program.
This is the behaviour for the DATABASE_URL. The app can not function without it.
I think that our case is less critical. The rest of the app can work. Only the re-index would not work.
I would only put the base URL and hardcode the path in the module since the path should not change. In a "near" future we should have a deployment that allows us to remove this extra host and just use the PHX_HOST value (should be dev.graasp.org, or graasp.org).
For setting the custom headers I like how they do it in the testing example docs: https://hexdocs.pm/req/Req.Test.html#module-example
This shows how the config is more code than just configuration.
The example also shows them splitting the "building the request" and "parsing the response" as two functions isolating them.
I hope these comments make sens and that they help you. Thank you again for the work, let me know if there is anything I can help with !
|
I applied changes on the code, not on the tests yet. |
|
I'm done 🌟 |
spaenleh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work !
I see that you have made the changes we talked about. I like how it works in the test, it is a lot cleaner (in regard to the application state and safe parallel execution).
I have opened a PR with the changes I think we should make and I have tested it. I think it was easier to do it this way, so I can show what I mean better than inside review comments: #118
I have made comments regarding naming. I propose to use:
Admin.Publications.SearchIndexConfig for the Config implementation
Admin.Publications.SearchIndexConfig.Behaviour for the config behaviour
Admin.Publications.SearchIndex for the module containing the reindexing code
SearchIndexConfigMock for the identifier used to Mock the behaviour in tests
We need to move the production configuration from prod.exs to runtime.exs so that the env var is taken at runtime and not at compile time.
I was curious about why we had the two hosts still and tested it to use the vite proxy on localhost:3114 but was not able to use it, even if a curl request was working ... not sure.
I removed the :publication_reindex_opts from the behaviour and made it environment dependent. I do not think we will need to change them between tests. I have set it to use the plug and added a retry: false so it does not retry in tests (makes them a lot faster).
I have changed the (stair)case in build_reindex_request to use pattern matching on function heads (having multiple function definitions with specific conditions on the arguments. I find it more readable, even if it is equivalent to having a if condition inside the body. Also I split the result and used the with to draw the "happy path" this way if we get an error with getting the url or the headers it will be returned. I think it is more readable like this, but let me know. I did not expect you to know these techniques, this is why I am showing them here.
I have used the .button component so it gets the tailwind styles (before there was no correct hover style).
That is all I think ! Thank you again for working on this and congrats on your first PR in Elixir !
|
@pyphilia Thank you for your review on my proposal. I will approve and merge this ! |
The reindex feature use the node.js backend to reindex the search index.
Next step would be to show the index status to keep track of the progress.
Screen.Recording.2025-12-15.at.12.00.58.mov
Screen.Recording.2025-12-15.at.12.02.28.mov
close #100