diff --git a/etsin_finder/app.py b/etsin_finder/app.py index f2b9567f0..696d6ac5f 100644 --- a/etsin_finder/app.py +++ b/etsin_finder/app.py @@ -65,6 +65,7 @@ def add_restful_resources(app): QvainDataset, QvainDatasets, QvainDatasetFiles, + QvainDatasetLock, FileCharacteristics, ) @@ -88,6 +89,7 @@ def add_restful_resources(app): api.add_resource(QvainDatasets, '/api/qvain/datasets') api.add_resource(QvainDataset, '/api/qvain/datasets/') api.add_resource(QvainDatasetFiles, '/api/qvain/datasets//files') + api.add_resource(QvainDatasetLock, '/api/qvain/datasets//lock') # Qvain API RPC endpoints api.add_resource(QvainDatasetChangeCumulativeState, '/api/rpc/datasets/change_cumulative_state') @@ -194,6 +196,7 @@ def create_app(testing=None): app.mail = Mail(app) app.cr_cache = CatalogRecordCache(app) app.cr_permission_cache = CatalogRecordCache(app, ttl=60, prefix='cr_permission_') + app.cr_lock_cache = CatalogRecordCache(app, ttl=60, prefix='cr_lock_', noreply=False) app.rems_cache = RemsCache(app) app.url_map.converters['id'] = IdentifierConverter diff --git a/etsin_finder/auth/authorization.py b/etsin_finder/auth/authorization.py index 0a90d96de..1d6b09007 100644 --- a/etsin_finder/auth/authorization.py +++ b/etsin_finder/auth/authorization.py @@ -7,8 +7,6 @@ """Functionalities related to authorization and what users are allowed to see.""" -from flask import current_app - from etsin_finder.auth import authentication from etsin_finder.services import cr_service from etsin_finder.services.cr_service import ( diff --git a/etsin_finder/cache.py b/etsin_finder/cache.py index dede297e8..5aa0270fb 100644 --- a/etsin_finder/cache.py +++ b/etsin_finder/cache.py @@ -12,6 +12,7 @@ from etsin_finder.app_config import get_memcached_config from etsin_finder.utils.utils import FlaskService +from etsin_finder.log import log class BaseCache(FlaskService): @@ -21,6 +22,7 @@ def __init__(self, app): """Init cache.""" super().__init__(app) + self.app = app memcached_config = get_memcached_config(app) if memcached_config: @@ -34,7 +36,7 @@ def __init__(self, app): elif not self.is_testing: app.logger.error("Unable to initialize Cache due to missing config") - def do_update(self, key, value, ttl): + def do_add(self, key, value, ttl, noreply=None): """Update cache. Update cache with new key and specific time-to-live. @@ -43,19 +45,42 @@ def do_update(self, key, value, ttl): key (str): The key to update. value (str): The value to update the key with. ttl (int): Number of seconds until the item is expired from the cache. + noreply (bool): Set True or False to override default noreply value for request. Returns: - str: The value. + bool: True if successful, otherwise False """ + success = False try: - self.cache.set(key, value, expire=ttl) + success = bool(self.cache.add(key, value, expire=ttl, noreply=noreply)) except Exception as e: - from etsin_finder.log import log + self.app.logger.warning("Add to cache failed") + self.app.logger.warning(e) + return success - log.warning("Insert to cache failed") - log.warning(e) - return value + def do_update(self, key, value, ttl, noreply=None): + """Update cache. + + Update cache with new key and specific time-to-live. + + Args: + key (str): The key to update. + value (str): The value to update the key with. + ttl (int): Number of seconds until the item is expired from the cache. + noreply (bool): Set True or False to override default noreply value for request. + + Returns: + bool: True if successful, otherwise False + + """ + success = False + try: + success = self.cache.set(key, value, expire=ttl, noreply=noreply) + except Exception as e: + self.app.logger.warning("Insert to cache failed") + self.app.logger.warning(e) + return success def do_get(self, key): """Try to fetch entry from cache. @@ -70,48 +95,92 @@ def do_get(self, key): try: return self.cache.get(key, None) except Exception as e: - from etsin_finder.log import log - - log.debug("Get from cache failed") - log.debug(e) + self.app.logger.debug("Get from cache failed") + self.app.logger.debug(e) return None - def do_delete(self, key): + def do_delete(self, key, noreply=None): """Try to delete entry from cache. Args: key (str): The key to fetch. + noreply (bool): Set True or False to override default noreply value for request. Returns: + bool: True if successful, otherwise False + + """ + success = False + try: + success = bool(self.cache.delete(key, noreply=noreply)) + except Exception as e: + self.app.logger.debug("Delete from cache failed") + self.app.logger.debug(e) + return success + + def do_gets(self, key): + """Try to fetch entry from cache for Check-And-Set. + + Args: + key (str): The key to fetch. + + Returns tuple: str: The value for the key, or default if the key wasn’t found. + token: CAS token """ try: - return self.cache.delete(key, None) + return self.cache.gets(key) except Exception as e: - from etsin_finder.log import log + self.app.logger.debug("Gets from cache failed") + self.app.logger.debug(e) + return None, None - log.debug("Delete from cache failed") - log.debug(e) - return None + def do_cas(self, key, value, token, ttl, noreply=None): + """Update cache only if it has not changed after gets + + Update cache with new key and specific time-to-live. + + Args: + key (str): The key to update. + value (str): The value to update the key with. + token (str): Token from do_gets. + ttl (int): Number of seconds until the item is expired from the cache. + noreply (bool): Set True or False to override default noreply value for request. + + Returns: + bool: True if successful, otherwise False + + """ + success = False + try: + success = bool( + self.cache.cas(key, value, token, expire=ttl, noreply=noreply) + ) + except Exception as e: + self.app.logger.warning("Insert to cache failed") + self.app.logger.warning(e) + return success class CatalogRecordCache(BaseCache): """Cache that stores data related to a specific catalog record.""" - def __init__(self, app, ttl=1200, prefix="cr_"): + def __init__(self, app, ttl=1200, noreply=None, prefix="cr_"): """Init cache. Args: app: App instance ttl: How long to store value, in seconds prefix: Cache key prefix, should be unique for each cache + noreply: Set noreply for all cache requests, None for defaults """ super().__init__(app) self.CACHE_ITEM_TTL = ttl self.CACHE_KEY_PREFIX = prefix + self.CACHE_NO_REPLY = noreply - def update_cache(self, cr_id, data): + def update(self, cr_id, data): """Update catalog record cache with catalog record json. Args: @@ -119,14 +188,19 @@ def update_cache(self, cr_id, data): data: Data to save. Returns: - data: Updated data + success (bool) """ if cr_id and data: - return self.do_update(self._get_cache_key(cr_id), data, self.CACHE_ITEM_TTL) + return self.do_update( + self._get_cache_key(cr_id), + data, + self.CACHE_ITEM_TTL, + noreply=self.CACHE_NO_REPLY, + ) return data - def get_from_cache(self, cr_id): + def get(self, cr_id): """Get data from catalog record cache. Args: @@ -135,19 +209,76 @@ def get_from_cache(self, cr_id): """ return self.do_get(self._get_cache_key(cr_id)) - def delete_from_cache(self, cr_id): + def delete(self, cr_id): """Delete data from catalog record cache. Args: cr_id (str): Catalog record identifier. Returns: - data: Saved data + success (bool) + + """ + return self.do_delete(self._get_cache_key(cr_id), noreply=self.CACHE_NO_REPLY) + + def gets(self, cr_id): + """Get value from cache. + + Args: + cr_id (str): Catalog record identifier. + data: Data to save. + + Returns: + data: Value from cache. + token: CAS token. + + """ + if cr_id: + return self.do_gets(self._get_cache_key(cr_id)) + return (None, None) + + def add(self, cr_id, data): + """Add value to cache. + + Args: + cr_id (str): Catalog record identifier. + data: Data to save. + + Returns: + success (bool) + + """ + if cr_id: + return self.do_add( + self._get_cache_key(cr_id), + data, + self.CACHE_ITEM_TTL, + noreply=self.CACHE_NO_REPLY, + ) + return False + + def cas(self, cr_id, data, token): + """Update catalog record cache with catalog record json. + + Args: + cr_id (str): Catalog record identifier. + data: Data to save. + + Returns: + success (bool) """ - return self.do_delete(self._get_cache_key(cr_id)) + return self.do_cas( + self._get_cache_key(cr_id), + data, + token, + self.CACHE_ITEM_TTL, + noreply=self.CACHE_NO_REPLY, + ) def _get_cache_key(self, cr_id): + if not cr_id: + log.warning("missing cr_id: {cr_id}") return f"{self.CACHE_KEY_PREFIX}{cr_id}" @@ -156,7 +287,7 @@ class RemsCache(BaseCache): CACHE_ITEM_TTL = 300 - def update_cache(self, cr_id, user_id, permission=False): + def update(self, cr_id, user_id, permission=False): """Update cache with user entitlement for a specific catalog record. Args: @@ -174,7 +305,7 @@ def update_cache(self, cr_id, user_id, permission=False): ) return permission - def get_from_cache(self, cr_id, user_id): + def get(self, cr_id, user_id): """Get entitlement for a user related to a specific catalog record from cache. Args: diff --git a/etsin_finder/frontend/__tests__/combined/qvain/a11y/qvain.datasets.a11y.test.jsx b/etsin_finder/frontend/__tests__/combined/qvain/a11y/qvain.datasets.a11y.test.jsx index 96e92ff38..dd72cb275 100644 --- a/etsin_finder/frontend/__tests__/combined/qvain/a11y/qvain.datasets.a11y.test.jsx +++ b/etsin_finder/frontend/__tests__/combined/qvain/a11y/qvain.datasets.a11y.test.jsx @@ -27,11 +27,8 @@ beforeEach(() => { }) }) -const datasetsCalls = observable.array([]) - jest.mock('axios') axios.get = jest.fn((...args) => { - datasetsCalls.push(JSON.parse(JSON.stringify(args))) return Promise.resolve({ data: datasets, }) @@ -41,7 +38,6 @@ describe('Qvain datasets page', () => { let wrapper beforeEach(async () => { - datasetsCalls.clear() stores.QvainDatasets.setDatasetsPerPage(5) wrapper = mount( @@ -54,8 +50,7 @@ describe('Qvain datasets page', () => { ) - // wait until datasets have been fetched - await when(() => datasetsCalls.length > 0) + await when(() => stores.QvainDatasets.datasetGroupsOnPage.length > 0) wrapper.update() // show more versions @@ -90,7 +85,6 @@ describe('Qvain dataset removal modal', () => { document.body.appendChild(helper) ReactModal.setAppElement(helper) - datasetsCalls.clear() wrapper = mount( diff --git a/etsin_finder/frontend/__tests__/components/qvain/qvain.datasets.datasetActions.test.js b/etsin_finder/frontend/__tests__/components/qvain/qvain.datasets.datasetActions.test.js index b937cf9fb..05bb1ef8f 100644 --- a/etsin_finder/frontend/__tests__/components/qvain/qvain.datasets.datasetActions.test.js +++ b/etsin_finder/frontend/__tests__/components/qvain/qvain.datasets.datasetActions.test.js @@ -50,8 +50,8 @@ const datasetWithChanges = { describe('getEnterEditAction', () => { describe.each([ - ['IDA', idaDataset, 'published-id', true], - ['draft', draftDataset, 'draft-id', true], + ['IDA', idaDataset, 'published-id', false], + ['draft', draftDataset, 'draft-id', false], ['changed', datasetWithChanges, 'changes-id', false], ])('given %s dataset', (description, dataset, expectedIdentifier, expectCallEditDataset) => { beforeEach(() => { diff --git a/etsin_finder/frontend/__tests__/components/qvain/qvain.main.test.jsx b/etsin_finder/frontend/__tests__/components/qvain/qvain.main.test.jsx index 2d0acb1f5..aa947342b 100644 --- a/etsin_finder/frontend/__tests__/components/qvain/qvain.main.test.jsx +++ b/etsin_finder/frontend/__tests__/components/qvain/qvain.main.test.jsx @@ -55,6 +55,7 @@ describe('given required props', () => { }, Env: { getQvainUrl: jest.fn(), + Flags: { flagEnabled: () => false }, }, } diff --git a/etsin_finder/frontend/__tests__/stores/view/qvain.lock.test.js b/etsin_finder/frontend/__tests__/stores/view/qvain.lock.test.js new file mode 100644 index 000000000..4d39d5b76 --- /dev/null +++ b/etsin_finder/frontend/__tests__/stores/view/qvain.lock.test.js @@ -0,0 +1,196 @@ +import 'chai/register-expect' +import axios from 'axios' +import { computed, observable, makeObservable, override, runInAction, when, action } from 'mobx' + +import LockClass from '../../../js/stores/view/qvain/qvain.lock' + +jest.mock('axios') +window.fetch = jest.fn() + +class FakeQvainClass { + // Helper class that allows changing dataset identifier, + // normally datasetIdentifier is a computed from original.identifier + + constructor() { + makeObservable(this, { datasetIdentifier: observable, setDatasetIdentifier: action }) + } + + datasetIdentifier = undefined + + setDatasetIdentifier(id) { + this.datasetIdentifier = id + } +} + +const Auth = { + userName: 'test_user', +} + +describe('things', () => { + const setup = () => { + jest.clearAllMocks() + jest.clearAllTimers() + jest.useFakeTimers('modern') + const Qvain = new FakeQvainClass() + const Lock = new LockClass(Qvain, Auth) + return { Qvain, Lock } + } + + const mockRequestLock = data => { + axios.put.mockReturnValue( + Promise.resolve({ + data, + }) + ) + } + + const mockRequestLockReject = (data, forceData) => { + axios.put.mockImplementation(async (url, { force }, hmm) => { + if (force) { + return Promise.resolve({ + data: forceData, + }) + } + throw { + response: { + data, + }, + } + }) + } + + describe('when lock is available for user', () => { + const setupLockAvailable = async () => { + const { Qvain, Lock } = setup() + Qvain.setDatasetIdentifier('datasetti') + mockRequestLock({ + cr_id: 'datasetti', + user_id: 'test_user', + }) + Lock.enable() + await when(() => !Lock.isPolling) + return { Lock } + } + + it('should get lock', async () => { + const { Lock } = await setupLockAvailable() + Lock.lockData.user.should.be.string('test_user') + Lock.haveLock.should.be.true + }) + + it('should keep polling', async () => { + const { Lock } = await setupLockAvailable() + axios.put.should.have.beenCalledTimes(1) + + for (let i = 2; i < 10; i++) { + Lock.isPolling.should.be.false + jest.advanceTimersByTime(Lock.pollInterval) + Lock.isPolling.should.be.true + await when(() => !Lock.isPolling) + axios.put.should.have.beenCalledTimes(i) + } + }) + }) + + describe('when another user has lock', () => { + const setupAnotherUserHasLock = async () => { + const { Qvain, Lock } = setup() + Qvain.setDatasetIdentifier('datasetti') + mockRequestLockReject( + { + cr_id: 'datasetti', + user_id: 'some_other_user', + }, + { + cr_id: 'datasetti', + user_id: 'test_user', + } + ) + Lock.enable() + await when(() => !Lock.isPolling) + return { Lock } + } + + it('should fail to get lock', async () => { + const { Lock } = await setupAnotherUserHasLock() + Lock.lockData.user.should.be.string('some_other_user') + Lock.haveLock.should.be.false + }) + + it('should keep polling', async () => { + const { Lock } = await setupAnotherUserHasLock() + axios.put.should.have.beenCalledTimes(1) + + for (let i = 2; i < 10; i++) { + jest.advanceTimersByTime(Lock.pollInterval) + await when(() => !Lock.isPolling) + axios.put.should.have.beenCalledTimes(i) + } + }) + + it('should acquire lock if using force', async () => { + const { Lock } = await setupAnotherUserHasLock() + await Lock.request({ force: true }) + Lock.lockData.user.should.be.string('test_user') + Lock.haveLock.should.be.true + }) + }) + + describe('when dataset changes', () => { + it('should release previous lock', async () => { + const { Qvain, Lock } = setup() + Qvain.setDatasetIdentifier('datasetti') + Lock.setLockData('datasetti', 'test_user') + Lock.enable() + Qvain.setDatasetIdentifier(undefined) + axios.delete.should.have.beenCalledWith('/api/qvain/datasets/datasetti/lock', { + timeout: Lock.requestTimeout, + }) + Lock.lockData.should.eqls({ dataset: undefined, user: undefined }) + }) + + it('should request new lock', async () => { + const { Qvain, Lock } = setup() + Lock.lockData = { + dataset: 'datasetti', + user: 'test_user', + } + Lock.enable() + Qvain.setDatasetIdentifier('datasetti') + axios.put.should.have.beenCalledWith( + '/api/qvain/datasets/datasetti/lock', + { force: false }, + { timeout: Lock.requestTimeout } + ) + }) + + it('should release lock with fetch when unload is called', () => { + const { Lock } = setup() + Lock.setLockData('datasetti', 'test_user') + Lock.enable() + Lock.unload() + expect(window.fetch).to.have.beenCalledWith('/api/qvain/datasets/datasetti/lock', { + keepalive: true, + method: 'DELETE', + }) + }) + }) + + describe('when no dataset is open', () => { + it('should not do anything', async () => { + const { Lock } = setup() + Lock.enable() + jest.advanceTimersByTime(Lock.pollInterval * 100) + Lock.pollingEnabled.should.be.false + axios.put.should.have.beenCalledTimes(0) + axios.delete.should.have.beenCalledTimes(0) + }) + + it('should not call fetch when unload is called', () => { + const { Lock } = setup() + Lock.enable() + Lock.unload() + expect(window.fetch).not.to.have.beenCalled() + }) + }) +}) diff --git a/etsin_finder/frontend/__tests__/stores/view/qvain.project.test.js b/etsin_finder/frontend/__tests__/stores/view/qvain.project.test.js index 86e139e69..62a5c16f7 100644 --- a/etsin_finder/frontend/__tests__/stores/view/qvain.project.test.js +++ b/etsin_finder/frontend/__tests__/stores/view/qvain.project.test.js @@ -28,10 +28,6 @@ describe('Projects', () => { test('should call makeObservable', () => { expect(makeObservable).to.have.beenCalledWith(projects) }) - - test('should set readonly', () => { - projects.readonly.should.be.false - }) }) describe('when calling reset', () => { diff --git a/etsin_finder/frontend/__tests__/utils/promiseManager.test.js b/etsin_finder/frontend/__tests__/utils/promiseManager.test.js index 0a5af2c32..cf1623f3a 100644 --- a/etsin_finder/frontend/__tests__/utils/promiseManager.test.js +++ b/etsin_finder/frontend/__tests__/utils/promiseManager.test.js @@ -16,8 +16,7 @@ describe('common.files.utils', () => { describe('when calling add', () => { beforeEach(() => { - testPromise = new Promise(jest.fn()) - promiseManager.add(testPromise) + testPromise = promiseManager.add(new Promise(jest.fn())) }) test('should add promise to promises', () => { @@ -39,9 +38,8 @@ describe('common.files.utils', () => { describe('when calling reset', () => { beforeEach(() => { - testPromise = new Promise(() => {}) + testPromise = promiseManager.add(new Promise(() => {})) testPromise.cancel = jest.fn() - promiseManager.add(testPromise) promiseManager.reset() }) diff --git a/etsin_finder/frontend/js/app.jsx b/etsin_finder/frontend/js/app.jsx index cf9f4fe18..74cb347d7 100644 --- a/etsin_finder/frontend/js/app.jsx +++ b/etsin_finder/frontend/js/app.jsx @@ -61,6 +61,9 @@ const App = () => { // Load runtime config const configure = async () => { await Env.fetchAppConfig() + if (Env?.Flags.flagEnabled('PERMISSIONS.WRITE_LOCK')) { + Stores.Qvain.Lock.enable() + } Locale.loadLang() hideSpinner() setInitialized(true) diff --git a/etsin_finder/frontend/js/components/qvain/fields/project/FundingAgency.jsx b/etsin_finder/frontend/js/components/qvain/fields/project/FundingAgency.jsx index 5dc053a3c..da6853d9a 100644 --- a/etsin_finder/frontend/js/components/qvain/fields/project/FundingAgency.jsx +++ b/etsin_finder/frontend/js/components/qvain/fields/project/FundingAgency.jsx @@ -148,7 +148,7 @@ const FundingAgencyForm = props => { const { onRemove, value } = props const { formData, addedFundingAgencies } = value - const { readonly } = props.Stores.Qvain.Projects + const { readonly } = props.Stores.Qvain const { lang } = props.Stores.Locale return ( @@ -158,6 +158,7 @@ const FundingAgencyForm = props => { onEdit={onEdit} onRemove={onRemove} lang={lang} + readonly={readonly} /> {' '} @@ -201,14 +202,16 @@ FundingAgencyForm.propTypes = { onChange: PropTypes.func.isRequired, } -const AddedAgencies = ({ agencies, onRemove, onEdit, lang }) => ( +const AddedAgencies = ({ agencies, onRemove, onEdit, lang, readonly }) => (
{agencies.map(agency => ( onEdit(agency.id)}> {agency.organization.organization.name[lang] || agency.organization.organization.name.und} - onRemove(agency.id)} icon={faTimes} size="xs" /> + {!readonly && ( + onRemove(agency.id)} icon={faTimes} size="xs" /> + )} ))}
@@ -219,10 +222,12 @@ AddedAgencies.propTypes = { onRemove: PropTypes.func.isRequired, onEdit: PropTypes.func.isRequired, lang: PropTypes.string.isRequired, + readonly: PropTypes.bool, } AddedAgencies.defaultProps = { agencies: [], + readonly: false, } /** diff --git a/etsin_finder/frontend/js/components/qvain/fields/project/FundingOrganization.jsx b/etsin_finder/frontend/js/components/qvain/fields/project/FundingOrganization.jsx index e4f11a7f7..de28e4c1c 100644 --- a/etsin_finder/frontend/js/components/qvain/fields/project/FundingOrganization.jsx +++ b/etsin_finder/frontend/js/components/qvain/fields/project/FundingOrganization.jsx @@ -24,7 +24,8 @@ import { useStores } from '../../utils/stores' const FundingOrganization = props => { const { Qvain: { - Projects: { orgObjectSchema, readonly }, + readonly, + Projects: { orgObjectSchema }, }, Locale: { lang }, } = useStores() diff --git a/etsin_finder/frontend/js/components/qvain/fields/project/ProjectForm.jsx b/etsin_finder/frontend/js/components/qvain/fields/project/ProjectForm.jsx index 3888d733b..9cf38db11 100644 --- a/etsin_finder/frontend/js/components/qvain/fields/project/ProjectForm.jsx +++ b/etsin_finder/frontend/js/components/qvain/fields/project/ProjectForm.jsx @@ -1,6 +1,7 @@ import React from 'react' import PropTypes from 'prop-types' +import { observer } from 'mobx-react' import Translate from 'react-translate-component' import t from 'counterpart' @@ -114,4 +115,4 @@ ProjectLabel.defaultProps = { required: false, } -export default ProjectForm +export default observer(ProjectForm) diff --git a/etsin_finder/frontend/js/components/qvain/fields/project/index.jsx b/etsin_finder/frontend/js/components/qvain/fields/project/index.jsx index 997c90c8e..a382f605b 100644 --- a/etsin_finder/frontend/js/components/qvain/fields/project/index.jsx +++ b/etsin_finder/frontend/js/components/qvain/fields/project/index.jsx @@ -224,7 +224,7 @@ class Project extends Component { removeProject = (id, event) => { if (event) event.preventDefault() - if (this.props.Stores.Qvain.Projects.readonly) return + if (this.props.Stores.Qvain.readonly) return if (id === this.state.id) this.resetForm() this.props.Stores.Qvain.Projects.removeProject(id) } @@ -242,7 +242,7 @@ class Project extends Component { render() { const { details, organizations, projectInEdit, fundingAgencies } = this.state - const { readonly } = this.props.Stores.Qvain.Projects + const { readonly } = this.props.Stores.Qvain return (
@@ -285,7 +285,8 @@ class Project extends Component { const AddedProjectsComponent = ({ Stores, editProject, removeProject }) => { const { lang } = Stores.Locale - const { projects, readonly } = Stores.Qvain.Projects + const { projects } = Stores.Qvain.Projects + const { readonly } = Stores.Qvain const renderProjectTitle = details => { const { fi, en } = details.title || {} diff --git a/etsin_finder/frontend/js/components/qvain/general/buttons/buttonContainers.jsx b/etsin_finder/frontend/js/components/qvain/general/buttons/buttonContainers.jsx index d46060f7d..0fbe0e41c 100644 --- a/etsin_finder/frontend/js/components/qvain/general/buttons/buttonContainers.jsx +++ b/etsin_finder/frontend/js/components/qvain/general/buttons/buttonContainers.jsx @@ -10,7 +10,7 @@ export const ButtonGroup = styled.div` border-radius: 4px; background-color: #fff; margin-bottom: 12px; - overflow: overlay; + overflow: hidden; > ${ButtonContainer} { margin-top: 0; diff --git a/etsin_finder/frontend/js/components/qvain/general/header/tooltipHoverOnSave.jsx b/etsin_finder/frontend/js/components/qvain/general/header/tooltipHoverOnSave.jsx index 91c4b2e7f..713289ee0 100644 --- a/etsin_finder/frontend/js/components/qvain/general/header/tooltipHoverOnSave.jsx +++ b/etsin_finder/frontend/js/components/qvain/general/header/tooltipHoverOnSave.jsx @@ -20,8 +20,10 @@ const TooltipHoverOnSave = ({ isOpen, children, errors, description }) => { const wrapperTooltipButtonRef = useRef(null) const wrapperTooltipCardRef = useRef(null) + const childSpan = {children} + if (!isOpen || errors.length === 0) { - return children + return childSpan } const TranslatedErrors = errors.map(error => @@ -34,7 +36,7 @@ const TooltipHoverOnSave = ({ isOpen, children, errors, description }) => { return ( <> - {children} + {childSpan} diff --git a/etsin_finder/frontend/js/components/qvain/general/input/organizationSelect.jsx b/etsin_finder/frontend/js/components/qvain/general/input/organizationSelect.jsx index cee104e62..195ba75c6 100644 --- a/etsin_finder/frontend/js/components/qvain/general/input/organizationSelect.jsx +++ b/etsin_finder/frontend/js/components/qvain/general/input/organizationSelect.jsx @@ -420,7 +420,7 @@ const CreatableSelectComponent = ({ {renderForm()}
- {allowReset ? ( + {allowReset && !readonly ? ( { const { - Qvain: { editDataset }, Env: { getQvainUrl, history }, Matomo, } = Stores @@ -18,7 +17,6 @@ export const getEnterEditAction = (Stores, dataset) => { } Matomo.recordEvent(`EDIT / ${dataset.identifier}`) - editDataset(dataset) history.push(getQvainUrl(`/dataset/${dataset.identifier}`)) }, } diff --git a/etsin_finder/frontend/js/components/qvain/views/datasets/table.jsx b/etsin_finder/frontend/js/components/qvain/views/datasets/table.jsx index dd48e3742..7f9b004b4 100644 --- a/etsin_finder/frontend/js/components/qvain/views/datasets/table.jsx +++ b/etsin_finder/frontend/js/components/qvain/views/datasets/table.jsx @@ -25,6 +25,8 @@ export class DatasetTable extends Component { componentDidMount() { const { loadDatasets } = this.props.Stores.QvainDatasets + const { resetQvainStore } = this.props.Stores.Qvain + resetQvainStore() loadDatasets() reaction( () => this.props.Stores.Auth.user.name, diff --git a/etsin_finder/frontend/js/components/qvain/views/editor/lockNotification.jsx b/etsin_finder/frontend/js/components/qvain/views/editor/lockNotification.jsx new file mode 100644 index 000000000..947382e70 --- /dev/null +++ b/etsin_finder/frontend/js/components/qvain/views/editor/lockNotification.jsx @@ -0,0 +1,62 @@ +import React from 'react' +import Translate from 'react-translate-component' +import styled from 'styled-components' +import { observer } from 'mobx-react' + +import { useStores } from '../../utils/stores' +import Loader from '../../../general/loader' +import Button from '../../../general/button' + +const LockNotification = () => { + const { + Qvain: { Lock, original }, + } = useStores() + + if (!Lock.enabled || !original || Lock.haveLock || Lock.isInitialLoad) { + return null + } + + const editButton = ( + Lock.request({ force: true })}> + + {Lock.isLoadingForced && } + + ) + + if (Lock.lockUser) { + return ( + + + {editButton} + + ) + } + + return ( + + + {editButton} + + ) +} + +const EditButton = styled(Button)` + display: inline-flex; + flex-wrap: no-wrap; + gap: 0.5em; + align-items: center; +` + +const LockText = styled.div` + background-color: ${props => props.theme.color.primaryLight}; + text-align: center; + width: 100%; + color: ${props => props.theme.color.primaryDark}; + z-index: 2; + border-bottom: 1px solid rgba(0, 0, 0, 0.3); + position: relative; + min-width: 300px; + padding: 0.25em; +` + +export default observer(LockNotification) diff --git a/etsin_finder/frontend/js/components/qvain/views/editor/stickyHeader.jsx b/etsin_finder/frontend/js/components/qvain/views/editor/stickyHeader.jsx index e66f7cb0e..19c928b3b 100644 --- a/etsin_finder/frontend/js/components/qvain/views/editor/stickyHeader.jsx +++ b/etsin_finder/frontend/js/components/qvain/views/editor/stickyHeader.jsx @@ -6,6 +6,7 @@ import { faChevronLeft } from '@fortawesome/free-solid-svg-icons' import { FontAwesomeIcon } from '@fortawesome/react-fontawesome' import DeprecatedState from './deprecatedState' import PasState from './pasState' +import LockNotification from './lockNotification' import SubmitButtons from './submitButtons' import { StickySubHeaderWrapper, @@ -70,6 +71,7 @@ const StickyHeader = ({ datasetError, submitButtonsRef, hideSubmitButtons }) => {!hideSubmitButtons && } + {submitted ? ( diff --git a/etsin_finder/frontend/js/components/qvain/views/main/index.jsx b/etsin_finder/frontend/js/components/qvain/views/main/index.jsx index 65fd89e60..f6b16f494 100644 --- a/etsin_finder/frontend/js/components/qvain/views/main/index.jsx +++ b/etsin_finder/frontend/js/components/qvain/views/main/index.jsx @@ -65,6 +65,9 @@ export class Qvain extends Component { window.removeEventListener('beforeunload', confirmReload) } }) + + // Attempt to release lock when window is closed, won't work on mobile + window.addEventListener('unload', this.props.Stores.Qvain.Lock?.unload) } componentDidUpdate(prevProps) { @@ -79,6 +82,8 @@ export class Qvain extends Component { if (this.disposeConfirmReload) { this.disposeConfirmReload() } + + window.removeEventListener('unload', this.props.Stores.Qvain.Lock?.unload) } async handleIdentifierChanged() { @@ -116,11 +121,14 @@ export class Qvain extends Component { getDataset(identifier) { this.setState({ datasetLoading: true, datasetError: false }) const { resetQvainStore, editDataset } = this.props.Stores.Qvain - const { getQvainUrl } = this.props.Stores.Env + const { + getQvainUrl, + Flags: { flagEnabled }, + } = this.props.Stores.Env const url = urls.qvain.dataset(identifier) const promise = axios .get(url) - .then(result => { + .then(async result => { resetQvainStore() // Open draft instead if it exists @@ -129,6 +137,13 @@ export class Qvain extends Component { this.props.history.replace(getQvainUrl(`/dataset/${nextDraft}`)) } else { editDataset(result.data) + if (flagEnabled('PERMISSIONS.WRITE_LOCK')) { + try { + await Promise.all(this.props.Stores.Qvain.Lock.promises) + // eslint-disable-next-line no-empty + } catch (e) {} + } + this.setState({ datasetLoading: false, datasetError: false, haveDataset: true }) } }) diff --git a/etsin_finder/frontend/js/stores/domain/auth.js b/etsin_finder/frontend/js/stores/domain/auth.js index f446f47b2..b9b976c79 100644 --- a/etsin_finder/frontend/js/stores/domain/auth.js +++ b/etsin_finder/frontend/js/stores/domain/auth.js @@ -8,7 +8,7 @@ * @license MIT */ -import { observable, action, runInAction, makeObservable } from 'mobx' +import { observable, action, computed, runInAction, makeObservable } from 'mobx' import axios from 'axios' class Auth { @@ -34,6 +34,10 @@ class Auth { isUsingRems: undefined, } + @computed get userName() { + return this.user?.name + } + @action resetUser = () => { this.user = { diff --git a/etsin_finder/frontend/js/stores/view/qvain/index.js b/etsin_finder/frontend/js/stores/view/qvain/index.js index 664ec9b1b..279ff6339 100644 --- a/etsin_finder/frontend/js/stores/view/qvain/index.js +++ b/etsin_finder/frontend/js/stores/view/qvain/index.js @@ -9,6 +9,7 @@ import { CUMULATIVE_STATE, DATA_CATALOG_IDENTIFIER } from '../../../utils/consta import Resources from './qvain.resources' import Files from './qvain.files' import Submit from './qvain.submit' +import Lock from './qvain.lock' import track, { touch } from './track' class Qvain extends Resources { @@ -19,6 +20,7 @@ class Qvain extends Resources { this.Submit = new Submit(this) this.resetQvainStore() makeObservable(this) + this.Lock = new Lock(this, Auth) } cumulativeStateSchema = cumulativeStateSchema @@ -137,6 +139,10 @@ class Qvain extends Resources { return result } + @computed get datasetIdentifier() { + return this.original?.identifier + } + @computed get getSelectedProject() { return this.selectedProject @@ -298,6 +304,11 @@ class Qvain extends Resources { @computed get readonly() { + if (this.Env?.Flags.flagEnabled('PERMISSIONS.WRITE_LOCK') && this.Lock.enabled) { + if (this.original && !this.Lock.haveLock) { + return true + } + } return ( this.preservationState >= 80 && this.preservationState !== 100 && diff --git a/etsin_finder/frontend/js/stores/view/qvain/qvain.lock.js b/etsin_finder/frontend/js/stores/view/qvain/qvain.lock.js new file mode 100644 index 000000000..1fb85a2d9 --- /dev/null +++ b/etsin_finder/frontend/js/stores/view/qvain/qvain.lock.js @@ -0,0 +1,193 @@ +import axios from 'axios' +import { observable, action, computed, makeObservable, reaction } from 'mobx' +import PromiseManager from '../../../utils/promiseManager' + +class Lock { + constructor(Qvain, Auth) { + this.Qvain = Qvain + this.Auth = Auth + makeObservable(this) + this.promiseManager = new PromiseManager() + this.poll = this.poll.bind(this) + this.handleDatasetChange = this.handleDatasetChange.bind(this) + this.unload = this.unload.bind(this) + reaction(() => this.Qvain.datasetIdentifier, this.handleDatasetChange) + } + + pollInterval = 30000 + + requestTimeout = 10000 + + enabled = false + + @observable lockData = {} + + @observable loading = false + + @observable isInitialLoad = false + + pollTimeoutId = null + + pollingEnabled = false + + handleDatasetChange(newIdentifier, oldIdentifier) { + if (!this.enabled) { + return + } + + if (this.haveLock && this.lockData?.dataset && this.lockData.dataset === oldIdentifier) { + this.stopPoll() + } + if (this.Qvain.datasetIdentifier) { + this.startPoll() + } + } + + @action async enable() { + this.enabled = true + if (this.Qvain.datasetIdentifier) { + await this.startPoll() + } + } + + @action async disable() { + await this.stopPoll() + this.enabled = false + } + + @computed get haveLock() { + return !!(this.lockData?.user === this.Auth?.userName) + } + + @computed get lockUser() { + return this.lockData?.user + } + + @computed get isLoading() { + return this.promiseManager.count('requestLock') > 0 + } + + @computed get isPolling() { + return this.promiseManager.count('pollLock') > 0 + } + + @computed get isLoadingForced() { + return this.promiseManager.count('requestLockForced') > 0 + } + + @computed get promises() { + return this.promiseManager.promises + } + + @action setLockData(dataset, user) { + this.lockData = { + dataset, + user, + } + } + + clearPollTimeout() { + clearTimeout(this.pollTimeoutId) + this.pollTimeoutId = null + } + + async startPoll() { + this.pollingEnabled = true + this.setInitialLoad(true) + this.clearPollTimeout() + this.poll() + } + + async stopPoll() { + this.pollingEnabled = false + this.setInitialLoad(false) + this.clearPollTimeout() + this.promiseManager.promises.forEach(p => p.cancel()) + await this.release() + } + + @action setInitialLoad(val) { + this.isInitialLoad = val + } + + async poll() { + const doPoll = async () => { + this.clearPollTimeout() + await this.request() + this.setInitialLoad(false) + if (this.pollingEnabled) { + this.pollTimeoutId = setTimeout(this.poll, this.pollInterval) + } + } + await this.promiseManager.add(doPoll(), 'pollLock') + } + + async request(options = {}) { + const dataset = this.Qvain.datasetIdentifier + if (!dataset || !this.enabled) { + return + } + const { force } = options + if (this.isLoading && !force) { + return + } + + const doRequest = async () => { + const data = { force: !!force } + let responseData + try { + const resp = await axios.put(`/api/qvain/datasets/${dataset}/lock`, data, { + timeout: this.requestTimeout, + }) + responseData = resp?.data + } catch (err) { + if (!err.response) { + console.warn(err) + } + if (err.response?.data?.user_id) { + responseData = err.response?.data + } + } + + if (responseData) { + this.setLockData(responseData.cr_id, responseData.user_id) + } + } + + const promiseTags = ['requestLock'] + if (force) { + promiseTags.push('requestLockForced') + } + + await this.promiseManager.add(doRequest(), ...promiseTags) + } + + async release() { + if (!this.enabled) { + return + } + const dataset = this.lockData?.dataset + this.setLockData(undefined, undefined) + if (dataset) { + await axios.delete(`/api/qvain/datasets/${dataset}/lock`, { + timeout: this.requestTimeout, + }) + } + } + + unload() { + // release lock on window unload + if (!this.enabled) { + return + } + const dataset = this.lockData?.dataset + if (dataset && window.fetch) { + window.fetch(`/api/qvain/datasets/${dataset}/lock`, { + keepalive: true, + method: 'DELETE', + }) + } + } +} + +export default Lock diff --git a/etsin_finder/frontend/js/stores/view/qvain/qvain.project.js b/etsin_finder/frontend/js/stores/view/qvain/qvain.project.js index 2c1840bee..9d70f7c02 100644 --- a/etsin_finder/frontend/js/stores/view/qvain/qvain.project.js +++ b/etsin_finder/frontend/js/stores/view/qvain/qvain.project.js @@ -51,7 +51,6 @@ export const projectSchema = yup.object().shape({ class Projects { constructor(Parent) { - this.readonly = Parent.readonly this.Parent = Parent makeObservable(this) } diff --git a/etsin_finder/frontend/js/stores/view/qvain/qvain.submit.js b/etsin_finder/frontend/js/stores/view/qvain/qvain.submit.js index d99b62d68..f33313dc4 100644 --- a/etsin_finder/frontend/js/stores/view/qvain/qvain.submit.js +++ b/etsin_finder/frontend/js/stores/view/qvain/qvain.submit.js @@ -126,10 +126,15 @@ class Submit { OtherIdentifiers: { cleanupBeforeBackend }, editDataset, setChanged, + Lock, } = this.Qvain this.response = undefined this.error = undefined + if (Lock) { + await Lock.request() + } + let draftFunction let publishFunction if (submitFunction.draftFunction || submitFunction.publishFunction) { diff --git a/etsin_finder/frontend/js/utils/promiseManager.js b/etsin_finder/frontend/js/utils/promiseManager.js index 6ba36e40f..0c1c2c746 100644 --- a/etsin_finder/frontend/js/utils/promiseManager.js +++ b/etsin_finder/frontend/js/utils/promiseManager.js @@ -14,23 +14,23 @@ export default class PromiseManager { count(tag = undefined) { if (tag !== undefined) { - return this.promisesWithTags.filter(({ tag: t }) => t === tag).length + return this.promisesWithTags.filter(({ tags }) => tags.includes(tag)).length } return this.promisesWithTags.length } // Keep track of promises and cancel all of them when reset() is called. // The canceled promises fail silently; resolve/reject callbacks of a canceled promise won't be called. - @action.bound add(promise, tag = undefined) { - this.promisesWithTags.push({ promise, tag }) - promise.finally( + @action.bound add(promise, ...tags) { + const promiseWithFinally = promise.finally( action(() => { this.promisesWithTags.replace( - this.promisesWithTags.filter(({ promise: p }) => p !== promise) + this.promisesWithTags.filter(({ promise: p }) => p !== promiseWithFinally) ) }) ) - return promise + this.promisesWithTags.push({ promise: promiseWithFinally, tags }) + return promiseWithFinally } @action.bound reset() { diff --git a/etsin_finder/frontend/locale/english/qvain.js b/etsin_finder/frontend/locale/english/qvain.js index 1d8d87c72..989a1a268 100644 --- a/etsin_finder/frontend/locale/english/qvain.js +++ b/etsin_finder/frontend/locale/english/qvain.js @@ -61,6 +61,11 @@ const qvainEnglish = { titleEdit: 'Edit dataset', titleLoading: 'Loading dataset', titleLoadingFailed: 'Loading dataset failed', + lock: { + force: 'Edit dataset', + error: 'Failed to open dataset for editing.', + unavailable: 'The dataset is being edited by %(user)s and is in read-only mode.', + }, error: { deprecated: 'Cannot publish dataset because it is deprecated. Please resolve deprecation first.', diff --git a/etsin_finder/frontend/locale/finnish/qvain.js b/etsin_finder/frontend/locale/finnish/qvain.js index 53fa91d79..1314b1ebe 100644 --- a/etsin_finder/frontend/locale/finnish/qvain.js +++ b/etsin_finder/frontend/locale/finnish/qvain.js @@ -17,8 +17,7 @@ const qvainFinnish = { edit: 'Päivitä aineisto', unsavedChanges: 'Sinulla on tallentamattomia muutoksia. Oletko varma että haluat poistua sivulta?', - consent: - `Käyttämällä Qvain -työkalua käyttäjä vakuuttaa, että hän on saanut suostumuksen + consent: `Käyttämällä Qvain -työkalua käyttäjä vakuuttaa, että hän on saanut suostumuksen muiden henkilöiden henkilötietojen lisäämiseen kuvailutietoihin ja ilmoittanut heille miten he voivat saada henkilötietonsa poistettua palvelusta. Käyttämällä Qvain -työkalua käyttäjä hyväksyy @@ -64,6 +63,11 @@ const qvainFinnish = { titleEdit: 'Muokkaa aineistoa', titleLoading: 'Ladataan aineistoa', titleLoadingFailed: 'Aineiston Lataus Epäonnistui', + lock: { + force: 'Muokkaa aineistoa', + error: 'Aineiston avaaminen muokattavaksi epäonnistui.', + unavailable: 'Aineisto on auki käyttäjällä %(user)s ja on vain luku -tilass.', + }, error: { deprecated: 'Aineistoa ei voida julkaista, koska aineisto on vanhentunut. Korjaa vanhentunut aineisto ensin.', diff --git a/etsin_finder/resources/qvain_resources.py b/etsin_finder/resources/qvain_resources.py index eb5f63a7f..1c93af02f 100644 --- a/etsin_finder/resources/qvain_resources.py +++ b/etsin_finder/resources/qvain_resources.py @@ -7,12 +7,14 @@ """RESTful API endpoints, meant to be used by Qvain form.""" +from etsin_finder.services.qvain_lock_service import QvainLockService from marshmallow import ValidationError from flask import request, current_app -from flask_restful import reqparse, Resource +from flask_restful import reqparse, Resource, abort from etsin_finder.auth import authentication from etsin_finder.log import log +from etsin_finder.utils.flags import flag_enabled from etsin_finder.utils.utils import datetime_to_header from etsin_finder.schemas.qvain_dataset_schema_v2 import ( @@ -23,6 +25,7 @@ from etsin_finder.utils.qvain_utils import ( data_to_metax, check_dataset_edit_permission, + check_dataset_edit_permission_and_lock, check_authentication, remove_deleted_datasets_from_results, edited_data_to_metax, @@ -254,7 +257,7 @@ def patch(self, cr_id): """ params = {} - error = check_dataset_edit_permission(cr_id) + error = check_dataset_edit_permission_and_lock(cr_id) if error is not None: return error @@ -320,7 +323,7 @@ def delete(self, cr_id): """ # only creator of the dataset is allowed to delete it - error = check_dataset_edit_permission(cr_id) + error = check_dataset_edit_permission_and_lock(cr_id) if error is not None: return error @@ -347,7 +350,7 @@ def post(self, cr_id): Metax response. """ - error = check_dataset_edit_permission(cr_id) + error = check_dataset_edit_permission_and_lock(cr_id) if error is not None: return error @@ -372,6 +375,51 @@ def post(self, cr_id): return response, status # adding or removing files may change permissions, clear them from cache - current_app.cr_permission_cache.delete_from_cache(cr_id) - + current_app.cr_permission_cache.delete(cr_id) return response, status + +class QvainDatasetLock(Resource): + """Endpoints for handling dataset write locks.""" + + def __init__(self): + """Initialization common for all methods""" + if not flag_enabled('PERMISSIONS.WRITE_LOCK'): + abort(405) + + def put(self, cr_id): + """Request/refresh write lock for dataset. + + The response status code is 200 if successful, 409 otherwise. + + Arguments: + cr_id {str} -- Identifier of dataset. + force {bool} -- Attempt to get lock regardless of its current holder. + + Returns: + lock {dict} -- lock object or {} if there is no lock currently + + """ + check_dataset_edit_permission(cr_id) + parser = reqparse.RequestParser() + parser.add_argument("force", type=bool, required=False) + args = parser.parse_args() + force = args.get("force", None) + + lock_service = QvainLockService() + success, data = lock_service.request_lock(cr_id, force) + + if success: + return data, 200 + return data, 409 + + def delete(self, cr_id): + """Release write lock for dataset. + + Arguments: + cr_id {str} -- Identifier of dataset. + + """ + check_dataset_edit_permission(cr_id) + lock_service = QvainLockService() + lock_service.release_lock(cr_id) + return "", 200 diff --git a/etsin_finder/resources/qvain_rpc.py b/etsin_finder/resources/qvain_rpc.py index 69519f2d3..76b69c13b 100644 --- a/etsin_finder/resources/qvain_rpc.py +++ b/etsin_finder/resources/qvain_rpc.py @@ -11,7 +11,7 @@ from etsin_finder.services.qvain_service import MetaxQvainAPIService from etsin_finder.utils.log_utils import log_request -from etsin_finder.utils.qvain_utils import check_dataset_edit_permission +from etsin_finder.utils.qvain_utils import check_dataset_edit_permission, check_dataset_edit_permission_and_lock class QvainDatasetChangeCumulativeState(Resource): @@ -38,7 +38,7 @@ def post(self): args = self.parser.parse_args() cr_id = args.get('identifier') cumulative_state = args.get('cumulative_state') - error = check_dataset_edit_permission(cr_id) + error = check_dataset_edit_permission_and_lock(cr_id) if error is not None: return error service = MetaxQvainAPIService() @@ -127,7 +127,7 @@ def post(self): """ args = self.parser.parse_args() cr_id = args.get('identifier') - err = check_dataset_edit_permission(cr_id) + err = check_dataset_edit_permission_and_lock(cr_id) if err is not None: return err service = MetaxQvainAPIService() @@ -156,7 +156,7 @@ def post(self): """ args = self.parser.parse_args() cr_id = args.get('identifier') - err = check_dataset_edit_permission(cr_id) + err = check_dataset_edit_permission_and_lock(cr_id) if err is not None: return err service = MetaxQvainAPIService() diff --git a/etsin_finder/services/cr_service.py b/etsin_finder/services/cr_service.py index 9ade4ac48..73c3ff52d 100644 --- a/etsin_finder/services/cr_service.py +++ b/etsin_finder/services/cr_service.py @@ -105,12 +105,15 @@ def get_catalog_record(cr_id, check_removed_if_not_exist, refresh_cache=False): """ if refresh_cache: - return current_app.cr_cache.update_cache(cr_id, _get_cr_from_metax(cr_id, check_removed_if_not_exist)) + cr = _get_cr_from_metax(cr_id, check_removed_if_not_exist) + current_app.cr_cache.update(cr_id, cr) + return cr - cr = current_app.cr_cache.get_from_cache(cr_id) + cr = current_app.cr_cache.get(cr_id) if cr is None: cr = _get_cr_from_metax(cr_id, check_removed_if_not_exist) - return current_app.cr_cache.update_cache(cr_id, cr) + current_app.cr_cache.update(cr_id, cr) + return cr else: return cr @@ -138,14 +141,15 @@ def get_perm(): perm = None if not refresh_cache: # try to get cached permissions - perm = current_app.cr_permission_cache.get_from_cache(cr_id) + perm = current_app.cr_permission_cache.get(cr_id) if perm is None: perm = get_perm() if perm: - return current_app.cr_permission_cache.update_cache(cr_id, perm) + current_app.cr_permission_cache.update(cr_id, perm) + return perm else: # getting permissions failed, remove previous entry from cache if any - current_app.cr_permission_cache.delete_from_cache(cr_id) + current_app.cr_permission_cache.delete(cr_id) return None else: return perm diff --git a/etsin_finder/services/qvain_lock_service.py b/etsin_finder/services/qvain_lock_service.py new file mode 100644 index 000000000..b71e631a0 --- /dev/null +++ b/etsin_finder/services/qvain_lock_service.py @@ -0,0 +1,79 @@ +"""Service for managing Qvain dataset write locks.""" + +from flask import request, current_app + +from etsin_finder.auth import authentication + + +class QvainLockService: + """Write locks for datasets in Qvain. + + Allows exclusive write access to a dataset for a short time. A lock + should be refreshed periodically to keep it from expiring. + + Lock objects are dicts containing: + { + "cr_id": Identifier of locked dataset + "user_id": User who currently owns the lock + } + """ + + def _get_lock_data(self, cr_id): + user_id = authentication.get_user_csc_name() + return {"cr_id": cr_id, "user_id": user_id} + + def request_lock(self, cr_id, force=False): + """Request/refresh lock for dataset. + + Uses Check-And-Set to confirm that the lock is available + before setting the new value atomically. + + Arguments: + cr_id {str} -- Identifier of dataset. + + Returns + bool -- True if lock is free now, False otherwise + + """ + data = self._get_lock_data(cr_id) + prev_data, token = current_app.cr_lock_cache.gets(cr_id) + + locked_by_other = prev_data and data != prev_data + success = False + if force or not locked_by_other: + if token: + success = current_app.cr_lock_cache.cas(cr_id, data, token) + else: + success = current_app.cr_lock_cache.add(cr_id, data) + if success: + return success, data + + return success, (prev_data or {}) + + def release_lock(self, cr_id): + """Release lock for dataset. + + Because the cache API does not support reading and deleting keys atomically, + this uses Check-And-Set to assign None value to the key. + + Arguments: + cr_id {str} -- Identifier of dataset. + + Returns + bool -- True if lock is free now, False otherwise + + """ + prev_data, token = current_app.cr_lock_cache.gets(cr_id) + if not token: + return True # lock already released + + data = self._get_lock_data(cr_id) + locked_by_other = (prev_data or token) and data != prev_data + if locked_by_other: + return False + + success = current_app.cr_lock_cache.cas(cr_id, None, token) + if not success: + return False + + return True diff --git a/etsin_finder/utils/flags.py b/etsin_finder/utils/flags.py index afea27c74..254b4cf15 100644 --- a/etsin_finder/utils/flags.py +++ b/etsin_finder/utils/flags.py @@ -19,6 +19,7 @@ "UI.SHOW_UNSUPPORTED", "UI.NEW_DATASETS_VIEW", "PERMISSIONS.SHARE_PROJECT", + "PERMISSIONS.WRITE_LOCK", "MATOMO_TRACKING", "CROSSREF_API", } diff --git a/etsin_finder/utils/qvain_utils.py b/etsin_finder/utils/qvain_utils.py index 9960ae50c..5430f24fe 100644 --- a/etsin_finder/utils/qvain_utils.py +++ b/etsin_finder/utils/qvain_utils.py @@ -6,7 +6,7 @@ from etsin_finder import auth from etsin_finder.utils.constants import DATA_CATALOG_IDENTIFIERS, ACCESS_TYPES -from etsin_finder.services import cr_service +from etsin_finder.services import cr_service, qvain_lock_service from etsin_finder.utils.flags import flag_enabled from etsin_finder.log import log @@ -222,6 +222,24 @@ def check_dataset_edit_permission(cr_id): }, 403 return None +def check_dataset_edit_permission_and_lock(cr_id): + """Check dataset permission and request write lock.""" + err = check_dataset_edit_permission(cr_id) + if err: + return err + + if flag_enabled('PERMISSIONS.WRITE_LOCK'): + lock_service = qvain_lock_service.QvainLockService() + success, data = lock_service.request_lock(cr_id) + if not success: + log.warning( + f"Failed to get lock for dataset {cr_id}." + ) + return { + "PermissionError": f"Dataset is locked for editing." + }, 409 + return None + def remove_deleted_datasets_from_results(result): """Remove datasets marked as removed from results. diff --git a/tests/test_cache.py b/tests/test_cache.py index 459e44de4..20187a62c 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -37,7 +37,7 @@ def mock_cache_set_error(self): def raise_(): raise Exception("set error.") - base.Client.set = lambda a, b, c, expire: raise_() + base.Client.set = lambda a, b, c, expire, noreply: raise_() @pytest.fixture def mock_cache_set(self, mocker): @@ -49,6 +49,23 @@ def mock_cache_get(self, mocker): """Mock cache.get function.""" mocker.patch("pymemcache.client.base.Client.get") + @pytest.fixture + def mock_cache_add(self, mocker): + """Mock cache.get function.""" + mocker.patch("pymemcache.client.base.Client.add", return_value=True) + + @pytest.fixture + def mock_cache_gets(self, mocker): + """Mock cache.gets function.""" + mocker.patch( + "pymemcache.client.base.Client.gets", return_value=("data", "token") + ) + + @pytest.fixture + def mock_cache_gets_fail(self, mocker): + """Mock cache.gets function.""" + mocker.patch("pymemcache.client.base.Client.gets", return_value=(None, None)) + @pytest.fixture def mock_cache_get_error(self): """Mock cache.get to throw an error.""" @@ -81,29 +98,35 @@ def test_base_cache_init(self, base_cache): def test_do_update(self, base_cache, mock_cache_set): """Call cache.set.""" base_cache.do_update("key", "value", 60) - base_cache.cache.set.assert_called_once_with("key", "value", expire=60) + base_cache.cache.set.assert_called_once_with( + "key", "value", expire=60, noreply=None + ) def test_do_update_fail(self, app, base_cache, mock_cache_set_error, expect_log): """Call cache.set and catch warnings.""" - with app.app_context(): - base_cache.do_update("key", "value", 60) - expect_log(warnings=["Insert to cache failed", "set error."]) + base_cache.do_update("key", "value", 60) + expect_log(warnings=["Insert to cache failed", "set error."]) def test_do_get(self, base_cache, mock_cache_get): """Call cache.get.""" base_cache.do_get("key") base_cache.cache.get.assert_called_once_with("key", None) + def test_do_gets(self, base_cache, mock_cache_gets): + """Call cache.gets.""" + (data, token) = base_cache.do_gets("key") + assert (data, token) == ("data", "token") + base_cache.cache.gets.assert_called_once_with("key") + def test_do_delete(self, base_cache, mock_cache_delete): """Call cache.delete.""" base_cache.do_delete("key") - base_cache.cache.delete.assert_called_once_with("key", None) + base_cache.cache.delete.assert_called_once_with("key", noreply=None) def test_do_delete_fail(self, app, base_cache, mock_cache_delete_error): """Call cache.delete and fail.""" - with app.app_context(): - result = base_cache.do_delete("key") - assert result is None + result = base_cache.do_delete("key") + assert result is False def test_cr_cache_init(self, catalog_record_cache): """Create a cache on init with default values.""" @@ -111,60 +134,87 @@ def test_cr_cache_init(self, catalog_record_cache): assert catalog_record_cache.CACHE_ITEM_TTL == 1200 assert catalog_record_cache.CACHE_KEY_PREFIX == "cr_" - def test_cr_update_cache(self, catalog_record_cache, mock_cache_set): + def test_cr_update(self, catalog_record_cache, mock_cache_set): """Call cache.set.""" cr_id = "id" data = "value" - catalog_record_cache.update_cache(cr_id, data) + catalog_record_cache.update(cr_id, data) catalog_record_cache.cache.set.assert_called_once_with( - "cr_id", "value", expire=1200 + "cr_id", "value", expire=1200, noreply=None ) - def test_cr_update_cache_cr_id_missing(self, catalog_record_cache): + def test_cr_update_cr_id_missing(self, catalog_record_cache): """Call cache.set.""" cr_id = None data = "value" - result = catalog_record_cache.update_cache(cr_id, data) + result = catalog_record_cache.update(cr_id, data) assert result == data - def test_cr_get_from_cache(self, catalog_record_cache, mock_cache_get): + def test_cr_get(self, catalog_record_cache, mock_cache_get): """Call cache.get.""" cr_id = "id" - catalog_record_cache.get_from_cache(cr_id) + catalog_record_cache.get(cr_id) catalog_record_cache.cache.get.assert_called_once_with("cr_id", None) - def test_cr_delete_from_cache(self, catalog_record_cache, mock_cache_delete): + def test_cr_delete(self, catalog_record_cache, mock_cache_delete): """Call cache.delete.""" cr_id = "id" - catalog_record_cache.delete_from_cache(cr_id) - catalog_record_cache.cache.delete.assert_called_once_with("cr_id", None) + catalog_record_cache.delete(cr_id) + catalog_record_cache.cache.delete.assert_called_once_with("cr_id", noreply=None) - def test_rems_update_cache(self, rems_cache, mock_cache_set): + def test_cr_add_to_cache(self, catalog_record_cache, mock_cache_add): + """Call cache.add.""" + cr_id = "id" + + catalog_record_cache.add(cr_id, "data") + catalog_record_cache.cache.add.assert_called_once_with( + "cr_id", "data", expire=1200, noreply=None + ) + + def test_cr_gets(self, catalog_record_cache, mock_cache_gets): + """Call cache.add.""" + cr_id = "id" + + data, token = catalog_record_cache.gets(cr_id) + assert (data, token) == ("data", "token") + catalog_record_cache.cache.gets.assert_called_once_with("cr_id") + + def test_cr_gets_fail(self, catalog_record_cache, mock_cache_gets_fail): + """Call cache.add.""" + cr_id = "id" + + data, token = catalog_record_cache.gets(cr_id) + assert (data, token) == (None, None) + catalog_record_cache.cache.gets.assert_called_once_with("cr_id") + + def test_rems_update(self, rems_cache, mock_cache_set): """Call cache.set.""" cr_id = "cr_" user_id = "id" permission = True - rems_cache.update_cache(cr_id, user_id, permission) - rems_cache.cache.set.assert_called_once_with("cr_id", True, expire=300) + rems_cache.update(cr_id, user_id, permission) + rems_cache.cache.set.assert_called_once_with( + "cr_id", True, expire=300, noreply=None + ) - def test_rems_update_cache_cr_id_missing(self, rems_cache, mock_cache_set): + def test_rems_update_cr_id_missing(self, rems_cache, mock_cache_set): """Call cache.set.""" cr_id = None user_id = "id" permission = True - result = rems_cache.update_cache(cr_id, user_id, permission) + result = rems_cache.update(cr_id, user_id, permission) assert result is True - def test_rems_get_from_cache(self, rems_cache, mock_cache_get): + def test_rems_get(self, rems_cache, mock_cache_get): """Call cache.get.""" cr_id = "cr_" user_id = "id" - rems_cache.get_from_cache(cr_id, user_id) + rems_cache.get(cr_id, user_id) rems_cache.cache.get.assert_called_once_with("cr_id", None) diff --git a/tests/test_qvain_lock.py b/tests/test_qvain_lock.py new file mode 100644 index 000000000..248f6433c --- /dev/null +++ b/tests/test_qvain_lock.py @@ -0,0 +1,127 @@ +"""Qvain dataset write lock tests""" + +import pytest + +from .basetest import BaseTest +from etsin_finder.utils.flags import set_flags, get_flags +from etsin_finder.app import add_restful_resources + + +class BaseLockTest(BaseTest): + """Base utilities for lock tests""" + + @pytest.fixture(autouse=True) + def _lock_app(self, app, mocker): + """Config common for all tests.""" + set_flags({"PERMISSIONS.WRITE_LOCK": True}, app) # enable write lock by default + mocker.patch( + "etsin_finder.resources.qvain_resources.check_dataset_edit_permission" + ) + + @pytest.fixture + def no_lock(self, mocker): + """Lock is not set.""" + return mocker.patch( + "pymemcache.client.base.Client.gets", return_value=(None, None) + ) + + success_lock_data = {"cr_id": "1337", "user_id": "teppo_testaaja"} + unavailable_lock_data = {"cr_id": "1337", "user_id": "someone_else"} + cas_token = "101" + + @pytest.fixture + def existing_lock(self, mocker): + """User has existing lock.""" + return mocker.patch( + "pymemcache.client.base.Client.gets", + return_value=( + self.success_lock_data, + self.cas_token, + ), + ) + + @pytest.fixture + def unavailable_lock(self, mocker): + """Another user has lock.""" + return mocker.patch( + "pymemcache.client.base.Client.gets", + return_value=( + self.unavailable_lock_data, + self.cas_token, + ), + ) + + @pytest.fixture + def add_success(self, mocker): + """Cache.add is successful.""" + return mocker.patch("pymemcache.client.base.Client.add", return_value=True) + + @pytest.fixture + def cas_success(self, mocker): + """Cache.cas is successful.""" + return mocker.patch("pymemcache.client.base.Client.cas", return_value=True) + + @pytest.fixture + def cas_fail(self, mocker): + """Cache.cas fails.""" + return mocker.patch("pymemcache.client.base.Client.cas", return_value=False) + + +class TestQvainLockRequest(BaseLockTest): + """Tests for requesting locks.""" + + def test_new_lock(self, authd_client, no_lock, add_success): + """Request new lock.""" + r = authd_client.put("/api/qvain/datasets/1337/lock") + assert r.json == self.success_lock_data + assert r.status_code == 200 + + def test_refresh_lock(self, authd_client, existing_lock, cas_success): + """Refresh existing lock.""" + r = authd_client.put("/api/qvain/datasets/1337/lock") + assert r.json == self.success_lock_data + assert r.status_code == 200 + + def test_refresh_fails(self, authd_client, existing_lock, cas_fail): + """Refreshing existing lock fails.""" + r = authd_client.put("/api/qvain/datasets/1337/lock") + assert r.json == self.success_lock_data + assert r.status_code == 409 + + def test_lock_unavailable(self, authd_client, unavailable_lock, add_success): + """Another user has lock, refreshing should fail.""" + r = authd_client.put("/api/qvain/datasets/1337/lock") + assert r.json == self.unavailable_lock_data + assert r.status_code == 409 + + def test_flag(self, app, authd_client): + """Flag is disabled, request should fail.""" + set_flags({"PERMISSIONS.WRITE_LOCK": False}, app) + r = authd_client.put("/api/qvain/datasets/1337/lock") + assert r.status_code == 405 + + +class TestQvainLockRelease(BaseLockTest): + """Tests for releasing locks.""" + + def test_release_lock(self, authd_client, existing_lock, cas_success): + """Lock value should be set to None.""" + r = authd_client.delete("/api/qvain/datasets/1337/lock") + cas_success.assert_called_once_with( + "cr_lock_1337", None, self.cas_token, expire=60, noreply=False + ) + assert r.status_code == 200 + + def test_released_already(self, authd_client, no_lock, cas_success): + """Lock is already released, lock should not be changed.""" + authd_client.delete("/api/qvain/datasets/1337/lock") + cas_success.assert_not_called() + + def test_release_lock_unavailable( + self, authd_client, unavailable_lock, cas_success + ): + """Someone else has lock, lock should not be changed.""" + r = authd_client.delete("/api/qvain/datasets/1337/lock") + unavailable_lock.assert_called_once_with("cr_lock_1337") + cas_success.assert_not_called() + assert r.status_code == 200 diff --git a/tests/test_qvain_utils.py b/tests/test_qvain_utils.py index 5e2a062ca..fd0ee55be 100644 --- a/tests/test_qvain_utils.py +++ b/tests/test_qvain_utils.py @@ -157,7 +157,7 @@ class TestCheckDatasetEditPermission(BaseTest): deniedOrNotExistResult = ({"PermissionError": "Dataset does not exist or user is not allowed to edit the dataset."}, 403) nonStandardCatalogResult = ({ - "PermissionError": f"Editing datasets from catalog nonstandard is not supported by Qvain." + "PermissionError": "Editing datasets from catalog nonstandard is not supported by Qvain." }, 403) @pytest.fixture