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

[ACA-4728] fix file order in viewer #3631

Conversation

g-jaskowski
Copy link
Contributor

@g-jaskowski g-jaskowski commented Feb 5, 2024

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation
  • Other... Please describe:

What is the current behaviour? (You can also link to an open issue here)
Files browsed in viewer with previous/next buttons have different order than in document list.
https://alfresco.atlassian.net/browse/ACA-4728

What is the new behaviour?
Getting file order in viewer is set accordingly to sorting mode and unified between pages and viewer.
Last sorting key is saved and used to prevent datatable rows changing order after closing viewer.
Folder structure is changed to reduce code duplication.

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

Copy link

sonarqubecloud bot commented Feb 5, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

64.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link
Contributor

Choose a reason for hiding this comment

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

@g-jaskowski @MichalKinas what do you think about creating single component instead of two different components (preview and viewer)?
Both components look almost the same, they have even similar unit tests. It would simplify maintenance in the future because changes would be required only in single component instead of in two.
This task is just bug fix so probably we would not like to do it here but maybe in separate task, what do you think? Would that be possible and good idea or better keep them as separate components?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might not be that easy as preview has another custom use case, when client has a custom plugin e.g. starting a new process he can use preview component to check if file that he wants to attach to the process is correct. We would need to check that use case and investigate if those can be merged then

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for response @MichalKinas. So should @g-jaskowski raise task to investigate that possibility and eventually merge these 2 components together if possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that's good idea to investigate that separately

Copy link
Contributor

Choose a reason for hiding this comment

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

great, then @g-jaskowski could you please raise task for this and put it to your team's board?

@AleksanderSklorz
Copy link
Contributor

@g-jaskowski before merging that PR please check sonarcloud issues

@g-jaskowski
Copy link
Contributor Author

g-jaskowski commented Feb 16, 2024

I added do not merge label for now, as additional fixes are needed

@g-jaskowski g-jaskowski force-pushed the ACA-4728-CLONE-ACA-Ordering-of-the-Next-Previous-buttons-in-the-file-viewer-does-not-correspond-to-the-sorting-order-of-the-document-list branch from 14c188c to 3be3ff7 Compare March 14, 2024 17:44
@g-jaskowski
Copy link
Contributor Author

@AleksanderSklorz @MichalKinas I tried to reduce code duplication with the refactor, it's still quite high, but can be lowered with some unit test optimization.
Also another small PR will be needed for ADW, but I wanted to make sure I took the right approach.

@g-jaskowski g-jaskowski force-pushed the ACA-4728-CLONE-ACA-Ordering-of-the-Next-Previous-buttons-in-the-file-viewer-does-not-correspond-to-the-sorting-order-of-the-document-list branch 2 times, most recently from 2a3f455 to 2a379fa Compare March 19, 2024 13:45
@g-jaskowski g-jaskowski force-pushed the ACA-4728-CLONE-ACA-Ordering-of-the-Next-Previous-buttons-in-the-file-viewer-does-not-correspond-to-the-sorting-order-of-the-document-list branch from d663fb3 to 41d2c31 Compare March 26, 2024 16:26
@g-jaskowski g-jaskowski force-pushed the ACA-4728-CLONE-ACA-Ordering-of-the-Next-Previous-buttons-in-the-file-viewer-does-not-correspond-to-the-sorting-order-of-the-document-list branch from ca6e3ac to 1561917 Compare March 28, 2024 13:09
@g-jaskowski g-jaskowski force-pushed the ACA-4728-CLONE-ACA-Ordering-of-the-Next-Previous-buttons-in-the-file-viewer-does-not-correspond-to-the-sorting-order-of-the-document-list branch from 1561917 to 0783b01 Compare April 4, 2024 08:42
Copy link

sonarqubecloud bot commented Apr 4, 2024

Quality Gate Passed Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
3.0% Duplication on New Code

See analysis details on SonarCloud

@g-jaskowski g-jaskowski merged commit 92a1e25 into develop Apr 10, 2024
26 checks passed
@g-jaskowski g-jaskowski deleted the ACA-4728-CLONE-ACA-Ordering-of-the-Next-Previous-buttons-in-the-file-viewer-does-not-correspond-to-the-sorting-order-of-the-document-list branch April 10, 2024 08:39
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.

4 participants