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

CV2-4072: Review fields of types TeamType and UserType #1770

Merged
merged 18 commits into from
Feb 1, 2024
Merged

CV2-4072: Review fields of types TeamType and UserType #1770

merged 18 commits into from
Feb 1, 2024

Conversation

melsawy
Copy link
Contributor

@melsawy melsawy commented Jan 3, 2024 •
edited
Loading

Description

User PublicTeamType for the following types:

  • FeedTeamType
  • FeedType
  • ProjectGroupType
  • ProjectType

Limit fields for UserType and add a new type MeType with current UserType fields

References: CV2-4072

How has this been tested?

Re-run automated tests.

Things to pay attention to during code review

Please describe parts of the change that require extra attention during code review, for example:

  • File FFFF, line LL: This refactoring does this and this. Is it consistent with how it’s implemented elsewhere?
  • Etc.

Checklist

  • I have performed a self-review of my own code
  • I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
  • I have added regression tests, if the PR fixes a bug
  • I have added logging, exception reporting, and custom tracing with any additional information required for debugging
  • I considered secure coding practices when writing this code. Any security concerns are noted above.
  • I have commented my code in hard-to-understand areas, if any
  • I have made needed changes to the README
  • My changes generate no new warnings
  • If I added a third party module, I included a rationale for doing so and followed our current guidelines

@melsawy melsawy changed the title CV2-4072: use PublicTeamType instead of TeamType CV2-4072: Review fields of types TeamType and UserType Jan 15, 2024
@melsawy melsawy marked this pull request as ready for review January 15, 2024 19:27
caiosba
caiosba requested changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

Thanks Sawy, I just noticed one more case: team_author in BotUserType should be PublicTeamType too. After this fix, fixes for the tests and changes on Check Web side, I think this is good to go :)

codeclimate[bot]
codeclimate bot reviewed Jan 22, 2024
View reviewed changes
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5930 lines exceeds the maximum allowed for the inline comments feature.

codeclimate[bot]
codeclimate bot reviewed Jan 25, 2024
View reviewed changes
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5930 lines exceeds the maximum allowed for the inline comments feature.

codeclimate[bot]
codeclimate bot reviewed Jan 29, 2024
View reviewed changes
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5924 lines exceeds the maximum allowed for the inline comments feature.

@melsawy melsawy requested a review from caiosba January 30, 2024 16:37
caiosba
caiosba reviewed Jan 30, 2024
View reviewed changes
app/models/user.rb Outdated
@@ -95,6 +95,10 @@ def self.from_token(token)
JSON.parse(Base64.decode64(token.gsub('++n', "\n")))
end

def me
self
Copy link
Contributor

Choose a reason for hiding this comment

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

@melsawy does it make sense to change this for:

User.current&.id == self.id ? self : nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes @caiosba makes sense and as you know I used this one as a return value for updateUser mutation so it'll return a value when current user did update action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@caiosba
Copy link
Contributor

caiosba commented Jan 30, 2024 •
edited
Loading

@melsawy also, please be sure it works on the Check Web side, specially with feed features (which are not covered by integration tests) - @danielevalverde can help you with testing this if needed.

@melsawy
Copy link
Contributor Author

melsawy commented Jan 31, 2024

@caiosba Already I tested the app locally especially features related to changed files.
also I back medias_count field to PublicTeamType with same way as other count fields like trash_count, unconfirmed_count, etc as without this field the item does not appear on item page immediately and require a page refresh as the item still assigned to a default project by default.
I think we can review these fields later after deprecate folders from the back-end

@melsawy melsawy requested a review from caiosba January 31, 2024 06:00
@codeclimate CodeClimate
Copy link

codeclimate bot commented Jan 31, 2024

Code Climate has analyzed commit f3832f2 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (100% is the threshold).

This pull request will bring the total coverage in the repository to 99.9%.

View more on Code Climate.

@melsawy melsawy merged commit 19b6e38 into develop Feb 1, 2024
8 checks passed
@melsawy melsawy deleted the CV2-4072-review-fields-of-types-team-type-and-user-type branch February 1, 2024 07:17
brianfleming added a commit that referenced this pull request Feb 8, 2024
@brianfleming
Revert "CV2-4072: Review fields of types TeamType and UserType (#…
1c58b79 
…1770)"

This reverts commit 19b6e38.
brianfleming added a commit that referenced this pull request Feb 8, 2024
@brianfleming
Revert "Revert "CV2-4072: Review fields of types TeamType and `User…
05a223b 
…Type` (#1770)""

This reverts commit 1c58b79.
brianfleming added a commit that referenced this pull request Feb 9, 2024
@brianfleming
User initiated conversations changed to service conversations (#1795)
70146dc 
* Revert "CV2-4072: Review fields of types `TeamType` and `UserType` (#1770)"

This reverts commit 19b6e38.

* re-label user conversations as service conversations per meta [CV2-4277]

* Revert "Revert "CV2-4072: Review fields of types `TeamType` and `UserType` (#1770)""

This reverts commit 1c58b79.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate[bot] left review comments

@caiosba caiosba caiosba approved these changes

@DGaffney DGaffney Awaiting requested review from DGaffney

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@melsawy @caiosba