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

Update API - notifications.json #5288

Closed
wants to merge 4 commits into from

Conversation

BWindey
Copy link
Contributor

@BWindey BWindey commented Jan 13, 2024

This pull request fixes the API returning a path for a variable called notifiable_url, in notifications.json.

Before:

[
  {
    "id": 17,
    ...
    "notifiable_url": "/nl/submissions/2441/#code"
  }
]

After:

[
  {
    "id": 17,
    ...
    "notifiable_path": "/nl/submissions/2441/#code",
    "notifiable_url": "http://dodona.localhost:3000/nl/submissions/2441.json#code"
  }
]

I'm not sure which one is preferred, the path or the url. I've noticed that paths are rarely used in the current state of the API, while url's appear often, if not always.

  • Tests were added
  • Documentation update can be found at dodona-edu/dodona-edu.github.io#

Closes #5286 .

@jorg-vr
Copy link
Contributor

jorg-vr commented Jan 15, 2024

This api variable might be used. These usecases might be broken by this update. Did you check for usages in the Dodona codebase?

We should probably also check for usecases in the VSCode and jetbrains plugins. I know at least one of them checks for notifications

@BWindey
Copy link
Contributor Author

BWindey commented Jan 15, 2024

I didn't think of the codebase using those functions. That is now fixed (it was used in 4 places, according to RubyMine).

I will see if I can find the usage of notifiable_url in the VSCode and Jetbrains plugins.

Edit:
thepieterdc's plugins for JetBrains don't appear to be using this.
xerbalind's plugin for neovim doesn't do anything with notifications.
gubogaer's plugin for Rstudio doesn't appear to do anything with notifications.
Dodona's plugin for creating exercises in VSCode doesn't appear to be using it either.

Maybe it is used in thepieterdc's plugin for VSCode, but the api-key (notifiable_url) is not called explicitly:

export function openNotifications() {
    const url = Uri.parse(`${getEnvironmentUrl()}/notifications`);
    commands.executeCommand("vscode.open", url);
}

@BWindey
Copy link
Contributor Author

BWindey commented Jan 17, 2024

I thought checks didn't run automatically when someone outside the Dodona team pushed a commit to their PR, and it's even on a draft PR?

@BWindey BWindey closed this Jan 21, 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.

API /notifications.json has field ..._url but shows ..._path
2 participants