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

Store and display publisher of package version #753

Closed
ericmj opened this issue Nov 27, 2018 · 14 comments

Comments

@ericmj
Copy link
Member

@ericmj ericmj commented Nov 27, 2018

Currently the published of a package is only stored in the audit log which is internal only.

  1. Store user who published release in database
  2. Show publisher on the website's package page
  3. Add to release API endpoint
  4. Add to mix hex.info PACKAGE VERSION
  5. Backfill release publishers from audit log
@edmz

This comment has been minimized.

Copy link
Contributor

@edmz edmz commented Nov 27, 2018

(I assume this is because of the event-stream fiasco, if not the please ignore.)

Could there be a way to notify the users of a package that a publisher has changed? Maybe when doing mix deps.get a package could be highlighted as one where the publisher has recently changed?

@ericmj

This comment has been minimized.

Copy link
Member Author

@ericmj ericmj commented Nov 27, 2018

We have been discussing that but the current thinking is that noise to value ratio will be too high.

@edmz

This comment has been minimized.

Copy link
Contributor

@edmz edmz commented Nov 27, 2018

Great to know you are thinking about it!

@pedroassumpcao

This comment has been minimized.

Copy link

@pedroassumpcao pedroassumpcao commented Nov 27, 2018

@ericmj is there anything planned in the future for searching by publisher so we should optimize the new column for it now?

@ericmj

This comment has been minimized.

Copy link
Member Author

@ericmj ericmj commented Nov 27, 2018

@pedroassumpcao I think it's enough to be able to search by owner for now so I would skip searching by publisher for now until we discover there is a need for it.

@dsdshcym

This comment has been minimized.

Copy link
Contributor

@dsdshcym dsdshcym commented Nov 28, 2018

Hi, @ericmj !

I want to work on this issue. Here is what I'm planning to do to "Store user who published release in database":

I assume there is only one publisher for every release, so I choose to add belongs_to :publisher, User relationship to Release schema. Please correct me if this assumption is wrong.

Here are my plans with some questions:

  1. Add release_publishers table (We write SQL directly in migration files, right?)
  2. Add belongs_to :publisher relationship to Repository.Release schema (belongs_to or has_many?)
  3. Add a test case for Releases.publish/7 to assert that it will set user as release.publisher. Change Releases.create_release/4 to Releases.create_release/5 and modify Release.build/3 to make the test pass
  4. Add validate_required :publisher_id to changeset(release, :update, params, package, checksum)

Please let me know your thoughts and if I can start working on this. Thanks!

@ericmj

This comment has been minimized.

Copy link
Member Author

@ericmj ericmj commented Nov 28, 2018

We can have a direct relation between User and Release with the belongs_to :publisher, User association on Release so we don't know need a release_publishers table. Everything else sounds good. 👍

@dsdshcym

This comment has been minimized.

Copy link
Contributor

@dsdshcym dsdshcym commented Nov 28, 2018

Cool! I'll just add publisher_id to releases table then.

It's 11pm now and I'm going to take some rest. I'll start working on this tomorrow and submit the PR ASAP.

Thanks for the quick reply!

dsdshcym added a commit to dsdshcym/hexpm that referenced this issue Nov 29, 2018
See also hexpm#753

1. Add `publisher_id` to `releases` table
2. Add db index for `publisher_id` in `releases` table
3. Add `belongs_to :publisher, User` relationship to Release schema
dsdshcym added a commit to dsdshcym/hexpm that referenced this issue Nov 29, 2018
See also hexpm#753

1. set publisher when a release is created
2. update publisher when a release is updated
dsdshcym added a commit to dsdshcym/hexpm that referenced this issue Nov 29, 2018
dsdshcym added a commit to dsdshcym/hex that referenced this issue Dec 22, 2018
@dsdshcym

This comment has been minimized.

Copy link
Contributor

@dsdshcym dsdshcym commented Dec 29, 2018

There's only "Show publisher on the website's package page" left unfinished now.

Here's my rough plan for adding this info:

  1. preload publisher in PackageController.package/6 for release
    release = Releases.preload(release, [:requirements, :downloads])
  2. add a publisher section (use the same design as owners) to show.html.eex
  3. extract a partial for rendering owner/publisher to remove the duplications
@ericmj

This comment has been minimized.

Copy link
Member Author

@ericmj ericmj commented Jan 4, 2019

I think we can close this now? Confirm /cc @dsdshcym @wojtekmach

@wojtekmach

This comment has been minimized.

Copy link
Member

@wojtekmach wojtekmach commented Jan 4, 2019

I think we have all the pieces, thanks @dsdshcym and let us know if you think anything else should be changed here!

@wojtekmach wojtekmach closed this Jan 4, 2019
@dsdshcym

This comment has been minimized.

Copy link
Contributor

@dsdshcym dsdshcym commented Jan 5, 2019

@ericmj @wojtekmach I think this feature is good now. When would it be deployed? I still can't see publishers for package releases on hex.pm.

And thanks a lot for your guidance during the development of this feature. 🙏

@ericmj

This comment has been minimized.

Copy link
Member Author

@ericmj ericmj commented Jan 7, 2019

Thank you @dsdshcym. It's going to be deployed today if we don't run into any issues.

@ericmj

This comment has been minimized.

Copy link
Member Author

@ericmj ericmj commented Jan 7, 2019

This has been deployed to production. I used the following migration to backfill publisher_id from the audit log.

UPDATE releases AS r
  SET publisher_id = al.user_id
  FROM audit_logs AS al
  WHERE al.action = 'release.publish'
    AND r.id = (al.params->'release'->>'id')::int;

This updated 35651 / 43023 releases. We can only backfill back to the introduction of the audit logs which was in early 2016.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.