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

Add support for showing all package versions. Refs #175. #208

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

nickygerritsen
Copy link
Contributor

In #175 there was a request to show the package versions. Since that is also a very useful feature for us, I have added this functionality.

Some notes / remarks:

  • I added a package details page so one can view the versions here. We could later possibly add more here (like the readme). I made anonymous organization users have access to it.
  • You can get to the details page both using the dropdown menu as well as clicking the package name.
  • Does showing the clipboard button make sense? For dev-* versions this is what you want but for other versions you might want to add ^ or ~ in front of it. Maybe only let one copy the version itself as well as a separate button to copy the package name?
  • I'm not sure if my logic in the Package and Version entity make sense?
  • Perhaps we should add a stable boolean to version so we can show the dev-* versions in a separate table?
  • I modified some tests to also test if we have the correct versions, but I'm not sure how/if we should test the reference as well? We could do it for the artifact case, since these ZIP's should have versions. For the local path one we can't assume anything since it uses local git info.

Some screenshots:
image
image

@nickygerritsen nickygerritsen force-pushed the package-details branch 9 times, most recently from e2a2eee to e59d5c5 Compare July 4, 2020 12:17
@codecov
Copy link

codecov bot commented Jul 4, 2020

Codecov Report

Merging #208 into master will decrease coverage by 0.06%.
The diff coverage is 96.73%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #208      +/-   ##
============================================
- Coverage     99.82%   99.75%   -0.07%     
- Complexity     1412     1439      +27     
============================================
  Files           231      234       +3     
  Lines          4001     4091      +90     
============================================
+ Hits           3994     4081      +87     
- Misses            7       10       +3     
Impacted Files Coverage Δ Complexity Δ
src/Service/Dist/Storage/FileStorage.php 93.61% <0.00%> (-6.39%) 15.00 <2.00> (+2.00) ⬇️
src/Controller/OrganizationController.php 100.00% <100.00%> (ø) 43.00 <1.00> (+1.00)
src/Entity/Organization/Package.php 100.00% <100.00%> (ø) 35.00 <7.00> (+6.00)
src/Entity/Organization/Package/Version.php 100.00% <100.00%> (ø) 9.00 <9.00> (?)
src/Query/User/Model/Version.php 100.00% <100.00%> (ø) 5.00 <5.00> (?)
src/Query/User/PackageQuery/DbalPackageQuery.php 100.00% <100.00%> (ø) 22.00 <2.00> (+2.00)
...ackageSynchronizer/ComposerPackageSynchronizer.php 98.57% <100.00%> (+0.21%) 24.00 <0.00> (ø)
src/Service/Twig/BytesExtension.php 100.00% <100.00%> (ø) 2.00 <2.00> (?)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b456d03...9801526. Read the comment docs.

src/Query/User/PackageQuery/DbalPackageQuery.php Outdated Show resolved Hide resolved
templates/organization/packages.html.twig Show resolved Hide resolved
src/Entity/Organization/Package.php Outdated Show resolved Hide resolved
templates/organization/package/details.html.twig Outdated Show resolved Hide resolved
@akondas
Copy link
Member

akondas commented Jul 6, 2020

Looks great to me, thanks for your time 🍻.

@nickygerritsen
Copy link
Contributor Author

Also a colleague of my suggested that it might be more logical to add a copy button for the composer require command. Shall I do the following two things:

  • Add a general copy button at the top that will composer require <packagename>; and
  • Change the copy button for each such that it will composer require <packagename>:<version>

?

@akondas
Copy link
Member

akondas commented Jul 6, 2020

Also a colleague of my suggested that it might be more logical to add a copy button for the composer require command. Shall I do the following two things:

  • Add a general copy button at the top that will composer require <packagename>; and
  • Change the copy button for each such that it will composer require <packagename>:<version>

?

This also sounds good 👍

@nickygerritsen
Copy link
Contributor Author

Fixes:

  • Added pagination. Wasn't too hard so why not 🤷‍♂️
  • Added size column
  • Changed logic as requested, although adding/updating versions is still in the synchronize method, size we need the distStorage
  • Updated the copy button things
  • Added more tests to have at least the same coverage as before 🎉

@nickygerritsen nickygerritsen force-pushed the package-details branch 4 times, most recently from de7fd7b to 95c2674 Compare July 6, 2020 19:40
@akondas
Copy link
Member

akondas commented Jul 7, 2020

@nickygerritsen After testing, I suggest you do one more thing: maybe we can add info when no versions are available:
Screenshot from 2020-07-07 08-11-13

To see available versions of the package, please perform synchronization again.

and button Sync now

Because currently all packages that have not been synchronized will have this page blank. The rest works fine. Good job 🍻

@akondas
Copy link
Member

akondas commented Jul 7, 2020

And what do you thing about something like this:
Screenshot from 2020-07-07 08-19-50
Light space compression. If it's better then I can add it to your PR.

@nickygerritsen
Copy link
Contributor Author

I will do the first thing then you can do the second, looks much better.

@nickygerritsen
Copy link
Contributor Author

nickygerritsen commented Jul 7, 2020

image

Tada, like this? Updated the code.

@akondas
Copy link
Member

akondas commented Jul 7, 2020

🤦 Actually, we don't really need this extra table, after all, all available versions can be fetch from the metadata file var/repo/organization/p/vendor/package.json. You do not need to change anything during synchronization. In general, it facilitates the whole process. Just write a parser that extracts this data from this file. Sorry, I don't know why I didn't think about it before ... 🤔

@nickygerritsen
Copy link
Contributor Author

🤦 Actually, we don't really need this extra table, after all, all available versions can be fetch from the metadata file var/repo/organization/p/vendor/package.json. You do not need to change anything during synchronization. In general, it facilitates the whole process. Just write a parser that extracts this data from this file. Sorry, I don't know why I didn't think about it before ... 🤔

But will it be fast enough? Also pagination is then less useful since we need to read and json_decode the whole file anyway? Or do you still want it for client-side render speed?

I can try to write a parser for this data.

@akondas
Copy link
Member

akondas commented Jul 7, 2020

Actually, that's right, I saw 30MB files, that would kill rendering this page. Let's stay with what you managed to do now. If this does not work, we can always change this 😉

@akondas
Copy link
Member

akondas commented Jul 7, 2020

This is my little UI refactor: https://github.com/Lets-Talk-NL/repman/pull/1/files 😉

@nickygerritsen
Copy link
Contributor Author

Nice, merged and squashed

@akondas akondas added this to the 0.4.0 milestone Jul 7, 2020
@akondas akondas merged commit 3dca2c2 into repman-io:master Jul 9, 2020
@akondas
Copy link
Member

akondas commented Jul 9, 2020

Thanks @nickygerritsen

@karniv00l karniv00l added the enhancement New feature or request label Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants