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.

Sign up for GitHub

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

Jump to bottom

Implementation of 'show related' functionality #1432 #1993

Merged
merged 33 commits into from
Jul 10, 2020
Merged

Implementation of 'show related' functionality #1432 #1993

merged 33 commits into from
Jul 10, 2020

Conversation

MarcelBolten
Copy link
Contributor

Hi Nicolas,
I tried to address the feature request as described in issue #1432.
For database items with many related items/experiments the long list of related entities might not be what a user wants and more features need to be added to reduce the amount.
However, I'm curious about your thoughts.

@MarcelBolten
Fix for Issue elabftw#707
e210c52
@MarcelBolten
Address CircleCI build errors
25b2fcd
@MarcelBolten
Merge pull request #2 from elabftw/hypernext
094b047 
Syncing from original
@MarcelBolten
Make CacheGenerator work with Windows OS
80c4039
@MarcelBolten
Merge pull request #10 from elabftw/hypernext
c3f534d 
Update to latest version on elabftw/hypernext
@MarcelBolten
Add functionality for 'show related' elabftw#1432
006cfc0
@MarcelBolten
Add functionality for 'show related' elabftw#1432
ddbac16
@MarcelBolten
Add functionality for 'show related' elabftw#1432
774c45b
@MarcelBolten
Fix Undefined variable for 'show related'
eeb1272
@MarcelBolten
Add functionality for 'show related' elabftw#1432
ee212e9 
Modify SQL query to list only related items and experiments for which the user has read permission.
@MarcelBolten
Update to make PR smooth
4dc349f
@MarcelBolten
Revert "Update to make PR smooth"
78e1a19 
This reverts commit 4dc349f.
@MarcelBolten
Merge branch 'hypernext' of https://github.com/elabftw/elabftw into h…
8c833e9 
…ypernext
@MarcelBolten
Related items/experiments elabftw#1432
61d3307 
Change variable and function names.
@MarcelBolten
Related items/experiments elabftw#1432
03c00f1 
Change function names.
@MarcelBolten
Related items/experiments elabftw#1432
0fb08e3 
Change variable names.
@MarcelBolten
Related items/experiments elabftw#1432
fc18f1e 
Simplify source code
@MarcelBolten
Revert "Make CacheGenerator work with Windows OS"
ca29da2 
This reverts commit 80c4039.
@NicolasCARPi
Copy link
Contributor

NicolasCARPi commented Jun 12, 2020 •
edited
Loading

Hello,

I don't like the code duplication that this creates. Maybe the query in Links should only get IDs and then you use the readShow() from AbstractEntity to get the data. Well it's not clear in my head how to do it, but it's important that the master query is only in one place.

@NicolasCARPi
Copy link
Contributor

also, this is just for view mode right? So you don't need to get all of that from the db, just what you'll display, category, name and id is enough.

@MarcelBolten
Copy link
Contributor Author

I agree with the unnecessary code duplication and combined the two functions readRelatedItemsAll() and readRelatedExperimentsAll() into a single function readRelated($type).

Regarding the complexity of the SQL query:
It only gets id, name, and category (for database items) but the query takes into account the read permission of the related entities.
I think this needs to be done to show only entities for which the user has read permission.

NicolasCARPi
NicolasCARPi requested changes Jun 12, 2020
View reviewed changes
Copy link
Contributor

@NicolasCARPi NicolasCARPi left a comment

Choose a reason for hiding this comment

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

I didn't run the code yet, but here are a few comments that you can address already. ;)

src/models/Links.php Outdated Show resolved Hide resolved
src/templates/steps-links-view.html Outdated Show resolved Hide resolved
src/templates/steps-links-view.html Show resolved Hide resolved
src/models/Links.php Outdated Show resolved Hide resolved
src/models/Links.php Outdated Show resolved Hide resolved
@MarcelBolten
Simplify SQL query by avoiding concatenation
beb1d84 
Use sprintf as e.g. in AbstractEntity->getReadSqlBeforeWhere()
@MarcelBolten
Merge pull request #16 from elabftw/hypernext
b9f8391 
RP from elabftw/elabftw
@MarcelBolten
fix indentation
2265e6c
@MarcelBolten
Remove readRelated argument and return array of arrays
d5a57c1 
Return an array that contains an array for experiments and one for items.
Uses associative array destructuring.
NicolasCARPi
NicolasCARPi reviewed Jun 15, 2020
View reviewed changes
src/models/Links.php Outdated Show resolved Hide resolved
@MarcelBolten
Copy link
Contributor Author

Hi Nicolas,
I guess your are busy with lots of other things. I just wanted to know if there is something more I can do on this PR or whether you want to run (more) tests before making a decision on how to move on here.
Cheers,
Marcel

@NicolasCARPi
Copy link
Contributor

Hello Marcel,

Sorry I kinda forgot this PR... It got drowned by the dependencies PRs... Thanks for reminding me. I'll look into it thoroughly asap!

@NicolasCARPi
Copy link
Contributor

Thank you for your contribution!

@NicolasCARPi NicolasCARPi merged commit 873cfb2 into elabftw:hypernext Jul 10, 2020
@MarcelBolten MarcelBolten deleted the show-related branch August 21, 2020 00:48
@fjbay fjbay mentioned this pull request Nov 27, 2020
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasCARPi NicolasCARPi NicolasCARPi approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MarcelBolten @NicolasCARPi