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

Scope all front-end requests where needed #3304

Merged
merged 11 commits into from
Jan 26, 2024
Merged

Scope all front-end requests where needed #3304

merged 11 commits into from
Jan 26, 2024

Conversation

maxpatiiuk
Copy link
Member

@maxpatiiuk maxpatiiuk commented Apr 5, 2023

Fixes #2292

To test:

  • Attachment counts are scoped to the current collection (can be checked by making sure the attachment count matches the number of displayed attachments in the attachment viewer) (they were not scoped before)
  • Pick lists of type "Entire table" or "Single Field" have their pick list items scoped (they were not scoped before, @grantfitzsimmons do you see a problem with this change?)

Make "domainFilter" parameter required in fetchCollection() to make it
harder to forget to specify correct value for that parameter (it
determines whether query is scoped).

Added "domainFilter:true" everywhere where necessary

Fixes #2292

You would notice that I added "domainFilter:false" in a lot of places
that seem like then need scoping. Example:

```js
fetchCollection('LoanPreparation', {
  isResolved: true,
  limit: DEFAULT_FETCH_LIMIT,
  preparation: preparation.get('id'),
  domainFilter: false,
})
```

This was done because the results would already be scoped correctly
because a filter on preparation id was added. In this case, setting
"domainFilter: true", won't make any difference, except that it adds
more filters to the query thus has performance overhead
@maxpatiiuk maxpatiiuk requested review from a team April 5, 2023 15:39
Triggered by 0b9a4d6 on branch refs/heads/issue-2292
@grantfitzsimmons
Copy link
Member

Pick lists of type "Entire table" or "Single Field" have their pick list items scoped (they were not scoped before, @grantfitzsimmons do you see a problem with this change?)

I do see potential problems with that change, as items that were once visible will no longer be (and previously valid pick list values would be invalid as well).

I'm not sure if this has a large scope, however, as it is not common to use pick-list items based on an entire table or field, but it might be worth asking Andy or Theresa.

Base automatically changed from issue-3291 to production April 5, 2023 17:30
Copy link
Contributor

@carlosmbe carlosmbe left a comment

Choose a reason for hiding this comment

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

Attachment Counts are still not scoped to current collection

Screen shots
Two different collections, same counts

Screenshot 2023-05-23 at 4 46 09 PM Screenshot 2023-05-23 at 4 46 46 PM

@maxpatiiuk
Copy link
Member Author

@melton-jason could you please pick up this PR?

Remaining things:

@maxpatiiuk maxpatiiuk self-assigned this Dec 23, 2023
@melton-jason
Copy link
Contributor

melton-jason commented Dec 26, 2023

Fix the issue discovered by Carlos

The problem here is that fetchCollection would always set the domainFilter property of the query to undefined (and thus default to false). This is because Specify could not resolve the scoping relationship of Attachment as it doesn't have a field titled collection, collectionobject, discipline, division, or institution. When the scoping relationship can not be resolved, the fetchCollection would always 'remove' the domainfilter.
Instead of being defined in a relationship, the 'scope' of an Attachment is defined in two columns on the Attachment table: scopetype (which is a number which represents/maps to the correct scoping level) and scopeid (the id of the scopetype).

The backend already has handling for scoping Attachments when domainfilter=true is provided:

if queryset.model is Attachment:
return queryset.filter(
Q(scopetype=None) |
Q(scopetype=scoping.GLOBAL_SCOPE) |
Q(scopetype=scoping.COLLECTION_SCOPE, scopeid=collection.id) |
Q(scopetype=scoping.DISCIPLINE_SCOPE, scopeid=collection.discipline.id) |
Q(scopetype=scoping.DIVISION_SCOPE, scopeid=collection.discipline.division.id) |
Q(scopetype=scoping.INSTITUTION_SCOPE, scopeid=collection.discipline.division.institution.id))

While the changes in 05ffff3 are 'hacky', I think any generic solution on the frontend would be quite messy (if not impossible without hardcoding around the attachment case).


Add a collection pref for making pick lists based on a table not scoped (like they were before this change) - the default should be scoped.

The collection preference can be added to the Remote Properties App Resource with the pattern: sp7_scope_table_picklists_<COLLECTION_ID>. (similar to the other Collection Preferences defined in this App Resource: CO_CREATE_COA, CO_CREATE_PREP, CO_CREATE_DET).
The default value is true, which means Specify will try and scope the picklist items by default.

Judging by the previous code and that in production, the picklistitems would only be unscoped (domainfilter set to false) when the table that the picklist is based off of is one of:

  • Collection
  • Discipline
  • Division
  • Institution

See

domainFilter: !f.includes(
Object.keys(schema.domainLevelIds),
toLowerCase(tableName)

This behavior has been preserved.

@melton-jason melton-jason requested review from a team January 15, 2024 15:25
@melton-jason melton-jason added this to the 7.9.4 milestone Jan 17, 2024
Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

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

Looks good, attachments and picklists seem to be scoped properly
Screenshot 2024-01-25 114034
Screenshot 2024-01-25 114104

Copy link
Contributor

@pashiav pashiav left a comment

Choose a reason for hiding this comment

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

Looks good! Could not reproduce the collection issue from earlier.

@melton-jason melton-jason merged commit 7324c28 into production Jan 26, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Some front-end requests are not properly scoped to current collection
7 participants