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

Pm search for packages - votes #14390

Merged
merged 15 commits into from
Oct 27, 2023

Conversation

dnenov
Copy link
Collaborator

@dnenov dnenov commented Sep 7, 2023

Purpose

Greg route working, now checks if user has voted for a package to prevent further voting. Current behavior is as follows:

  • users can vote for a package that has been installed
  • users can vote if they have signed in
  • users can vote if they haven't already voted for that package
  • if the above criteria is met, the user will be allowed to vote for a package through the details panel
  • the user will see visual indication for all voted packages

!!! Greg updated! The new request GetUserVotes to add the user/votes route that was introduced with this PR is still not showing on the Dynamo side.

This is a cherry-pick with package manager changes around the detailed package extension, and specifically the votes behavior. Alongside the visual updates, the PR changes the following - users will be able to vote for a package if they have it installed. Downloading is still disabled.

UI Changes

votes working

votes

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

  • adding changes connected to the 'votes' behavior
  • voting for packages functionality reinstated
  • checking for user votes on loading
  • if the user has already voted for a package prevents from voting again
  • ui updates

Reviewers

@zeusongit
@sm6srw

FYIs

@QilongTang

- adding changes connected to the 'votes' behavior
- still not showing the new 'GetUserVotes' request that was added with this PR DynamoDS/PackageManagerClient#84
@zeusongit
Copy link
Contributor

Sorry, I reviewed one commit(was routed through outlook) rather than the PR, will wait for the PR to be ready for review.

@QilongTang
Copy link
Contributor

hi @zeusongit, Deyan mentioned with the newer version of Greg, the new route does not exist still so this PR does not build

@zeusongit
Copy link
Contributor

hi @zeusongit, Deyan mentioned with the newer version of Greg, the new route does not exist still so this PR does not build

Hi @QilongTang , I tried it locally, i updated the greg package to 6084, and it is building for me successfully locally, one thing that I noticed is that maybe greg is not updated in all the locations where it is used, so do a complete update maybe?
Screenshot 2023-09-07 at 1 00 17 PM
Screenshot 2023-09-07 at 1 00 57 PM

@reddyashish
Copy link
Contributor

@dnenov Is this ready for review or any changes coming in?

- voting for packages functionality reinstated
- checking for user votes on loading
- if the user has already voted for a package prevents from voting again
- ui updates
- now correctly updates the SeachElement item inside the PackageManagerPackagesControl 'votes' icon when user has voted for a package
- now allows to vote for a package right after a package has been installed
@dnenov dnenov marked this pull request as ready for review October 22, 2023 10:26
@dnenov
Copy link
Collaborator Author

dnenov commented Oct 22, 2023

@dnenov Is this ready for review or any changes coming in?

Hey @reddyashish ! This one is definitely ready for review, it must have fallen under the cracks. My bad!

- removed duplicate package references
@@ -261,6 +261,21 @@ public void Upvote_ReturnsFalseWhenRequestThrowsException()
Assert.IsFalse(res);
}

//[Test]
Copy link
Contributor

Choose a reason for hiding this comment

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

Uncomment this test? would be nice to test this. Is the mock test failing for any reason?

return pkgResponse.content;
}, null);

return votes.has_upvoted;
Copy link
Contributor

Choose a reason for hiding this comment

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

the local variable votes will be null when the function fails. Check for null before returning its property.

@reddyashish
Copy link
Contributor

Some comments @dnenov. Also double check the additional references that are added. You would need to pull in the latest master again to resolve the conflicts.

- reconciled meshed up references
- comments
- last reference?
@@ -8,7 +8,7 @@
// associated with an assembly.
[assembly: AssemblyCompany("Autodesk, Inc")]
[assembly: AssemblyProduct("Dynamo")]
[assembly: AssemblyCopyright("Copyright © Autodesk, Inc 2023")]
[assembly: AssemblyCopyright("Copyright Autodesk, Inc 2023")]
Copy link
Contributor

Choose a reason for hiding this comment

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

This file shouldn't be added, that is why the self-serve is failing.

@reddyashish
Copy link
Contributor

3 failing tests on this PR. Can you take a look.
Screenshot 2023-10-25 at 1 10 46 PM

@dnenov
Copy link
Collaborator Author

dnenov commented Oct 25, 2023

3 failing tests on this PR. Can you take a look. Screenshot 2023-10-25 at 1 10 46 PM

Sure thing, and I will sort out the Assemblies entry.

- revert changes made by mistake
@QilongTang QilongTang added this to the 3.0 milestone Oct 26, 2023
@reddyashish reddyashish merged commit 8eee844 into DynamoDS:master Oct 27, 2023
17 checks passed
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