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

JENA-2282: Fuseki2 Query Store #1459

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

Conversation

jamiefeiss
Copy link

Jira issue: https://issues.apache.org/jira/browse/JENA-2282

Pull request Description:

An initial implementation of a query library for the Fuseki2 Vue UI using Vuex. Adds two buttons to the dataset query page for saving & loading queries. From there, users can save queries to a query library that persists in local storage in the browser.


  • Tests are included.
  • Documentation change and updates are provided for the Apache Jena website
  • Commits have been squashed to remove intermediate development commit messages.
  • Key commit messages start with the issue number (GH-xxxx or JENA-xxxx)

By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement.


See the Apache Jena "Contributing" guide.

@afs
Copy link
Member

afs commented Jul 28, 2022

Hi @jamiefeiss!

Great to see this!

It is showing as having a conflict with the main branch of the codebase. There is major maintenance PR #1307and we've made some version upgrades due to a security alert.

Your PR branch is showing as 77 commits behind main so it looks (please confirm) that it is just behind on main and isn't a PR relative to the Vue upgrade work.

@kinow what order of PRs should we work with?

We're approaching a release (early August?) - I'm neutral as to whether this goes in before or after. Mostly, Jena releases are done on a regular tick, and not feature driven. Features take the time they take!

@kinow
Copy link
Member

kinow commented Aug 2, 2022

@kinow what order of PRs should we work with?

If @jamiefeiss doesn't mind, I think it'd be easier to get the #1307 in first, as that one also adds the e2e tests. Later, if needed, I can help converting this PR from Bootstrap 4 to Bootstrap 5, and help with e2e tests as well. I haven't reviewed the code yet, but if it changes the Query editor we can add some tests first to main, and then confirm this change doesn't add any regression.

@jamiefeiss
Copy link
Author

Thanks @afs & @kinow.

I'm fine with waiting for the other PR first. Once the other PR is done, I'll get this PR up to date with main, update my code to use Bootstrap 5 and write some tests.

@afs
Copy link
Member

afs commented Aug 16, 2022

#1307 has merged.

I'd like to get a Jena release because it has been slipping for multiple reasons, none due to any UI work.

There are several PRs queued and we can't wait for them to complete because while that happens, others appear!

So I'll wait a few days then release what is ready at the time.

@afs
Copy link
Member

afs commented Nov 1, 2022

The Vue 3 migration work (issue #1306 PR #1480) has been completed!

Hopefully, that clears the way for this PR.

@kinow
Copy link
Member

kinow commented Nov 2, 2022

Happy to help with the conflicts if needed 👍

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Rebased onto main, updated libraries, fixed conflicts, and tested locally:

image

@jamiefeiss I can see the Save and Load option that was added 🙂

Unfortunately, besides Vue 3, we also ditched Bootstrap-Vue. So the new components need to be updated as well, to remove Bootstrap-Vue code (like the modal, failing in the screenshot above).

I did that not too long ago, so I think I might be able to convert it and finish testing this new feature 🎉 Probably in a few days (doing that during the breaks of the world cup matches 😬 ).

Cheers

Bruno

"vue": "^3.2.39",
"vue-router": "^4.1.5",
"vue-upload-component": "^3.1.2"
"vue-upload-component": "^3.1.2",
"vuex": "^4.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

Updated the version of Vuex as we are using Vue 3 now too.

import VuexPersistence from 'vuex-persist'
import { queryLibraryStore } from './queryLibraryStore'

export default createStore({
Copy link
Member

Choose a reason for hiding this comment

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

Used createStore (Vuex 4.x + Vue 3)

import 'bootstrap/dist/js/bootstrap.bundle.min'
import { FusekiServicePlugin, ToastPlugin } from '@/plugins/index'

const app = createApp(App)

app.use(router)
app.use(store)
Copy link
Member

Choose a reason for hiding this comment

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

@nicholascar
Copy link

@kinow does this PR need anything other than the obvious merge conflict to progress? You mentioned above (in 2022!) that you might "convert it and finish testing this new feature", could you still do that?

@kinow
Copy link
Member

kinow commented Oct 12, 2024

You mentioned above (in 2022!) that you might "convert it and finish testing this new feature", could you still do that?

Sorry the delay. I think there are no other pressing issues for Jena UI, so perfect time to get this one sorted out and merged. I can review the conflicts and push a separate commit with the conflict fixes and test it and maybe write tests if pending (or provide a skeleton if it's too complex to test). Then you can review my commit and we squash it later if the changes look good to you. Or if you prefer to fix the conflicts, that's fine by me too. I'll be overseas for 1 week, but have one week per month now to work on Jena (3 in Dec/Jan 😬 🎉 ), so quite sure we can get this reviewed/merged in one of these weeks/months, @nicholascar .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants