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

Automatic submission(s) page refreshing on status update #145

Closed
wants to merge 2 commits into from
Closed

Automatic submission(s) page refreshing on status update #145

wants to merge 2 commits into from

Conversation

etiaro
Copy link
Contributor

@etiaro etiaro commented Jan 31, 2023

Resolves #104
Musiałem minimalnie dostosować testy, bo dodanie nowej funkcji "status_registry" czasem zwiększa liczbę zapytań

@etiaro etiaro requested a review from twalen as a code owner January 31, 2023 16:35
@A-dead-pixel
Copy link
Contributor

Isn't the status fetched only every 5 minutes for non-superusers?
I think this would be much more useful if it utilised data from the notifications module.

@etiaro
Copy link
Contributor Author

etiaro commented Feb 1, 2023

Isn't the status fetched only every 5 minutes for non-superusers? I think this would be much more useful if it utilised data from the notifications module.

Right, I forgot to check how often it happens for standard users. I'll look into this notifications module

@etiaro
Copy link
Contributor Author

etiaro commented Feb 1, 2023

Isn't the status fetched only every 5 minutes for non-superusers? I think this would be much more useful if it utilised data from the notifications module.

Do you know how to get notifications working in local environment?
Or even how to get them working on szkopuł as regular user?

@A-dead-pixel
Copy link
Contributor

A-dead-pixel commented Feb 2, 2023

I will write the deployment instructions later today.
As for szkopuł, they are always on if you aren't viewing from your phone or logged out. They are the thing showed in #84. You access them by clicking the yellowish dropdown button on the navbar.

@etiaro
Copy link
Contributor Author

etiaro commented Feb 2, 2023

Got notifications working in really unconvienient way (set up and run rabbitmq and notifications_server in web container's bash), and they work alright, automatic refresh can be easily implemented.
I'm not sure if I should also refresh submissions list page?
Also, tried to replicate everything in Szkopuł, on never used account and browser, and it didn't work. Notification server is running and connecting, but there are no received notifications

@A-dead-pixel
Copy link
Contributor

Ideally, the new submission/result would be inserted into the list, otherwise refreshing the page should be decent. I'm not sure whether the other submissions for given problem below should refresh the page in the second case.

I checked, and it seems you're right: notifications on szkopuł are broken. Someone responsible should be informed.

The settings I use to enable notifications for local deployment:

  • expose port 7887 in docker-compose
  • settings.py:
NOTIFICATIONS_SERVER_ENABLED = True
NOTIFICATIONS_RABBITMQ_URL = 'amqp://oioioi:oioioi@broker'
NOTIFICATIONS_SERVER_URL = 'http://localhost:7887/'

Other defaults are fine I think. You don't have to run another rabbitmq instance.
This isn't exactly needed, but I also install nodejs and npm in the dockerfile, as opposed to leaving the installation to notifications_server.

@etiaro
Copy link
Contributor Author

etiaro commented Feb 2, 2023

Thanks! I'll go with implementing the automatic updates at the weekend.
I think it would be useful to add these instructions to notifications page at https://oioioi.readthedocs.io or at least change the default values in settings.py template, but I'm not sure if I know how to do that:
Will changing contents of this file and this file work?

@A-dead-pixel
Copy link
Contributor

I have no experience with readthedocs, but I think the notifications/README.rst should be updated instead, at least to mention the port exposure and to clear up what settings need to be changed. It seems that the readthedocs stuff is more directed at APIs and the inner workings of oioioi. Nevertheless, it wouldn't hurt to mention the README there.

I don't know where the default settings for the rabbitmq connection should be changed, but it would be safest to add it to the sed command in the Dockerfile, where other docker-specific configuration changes reside, though it isn't too consistent with the deprecation of non-docker deployments.
That aside, could you convert this PR to a draft for now?

@etiaro
Copy link
Contributor Author

etiaro commented Feb 3, 2023

Sure, tbh I'm gonna make another one because these changes are going to trash

@etiaro etiaro closed this Feb 3, 2023
@etiaro
Copy link
Contributor Author

etiaro commented Feb 5, 2023

Moved to #146

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 automatic submission page refresh
2 participants