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

regression: E2EE DM setup header toolbox items #32712

Merged
merged 14 commits into from
Jul 5, 2024

Conversation

yash-rajpal
Copy link
Member

@yash-rajpal yash-rajpal commented Jul 2, 2024

Proposed changes (including videos or screenshots)

The E2EE room setup header was showing irrelevant toolbox items on DM rooms which were not applicable for DMs.

Issue(s)

Steps to test or reproduce

Further comments

E2EE2-45

Copy link
Contributor

dionisio-bot bot commented Jul 2, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented Jul 2, 2024

⚠️ No Changeset found

Latest commit: 7b25dec

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Please upload report for BASE (release-6.10.0@dff6d18). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                @@
##             release-6.10.0   #32712   +/-   ##
=================================================
  Coverage                  ?   56.58%           
=================================================
  Files                     ?     2502           
  Lines                     ?    55538           
  Branches                  ?    11471           
=================================================
  Hits                      ?    31426           
  Misses                    ?    21420           
  Partials                  ?     2692           
Flag Coverage Δ
e2e 56.44% <90.00%> (?)
unit 71.59% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@yash-rajpal yash-rajpal added this to the 6.10 milestone Jul 2, 2024
@milton-rucks milton-rucks added the stat: QA assured Means it has been tested and approved by a company insider label Jul 3, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jul 3, 2024
@yash-rajpal yash-rajpal marked this pull request as ready for review July 4, 2024 13:36
@yash-rajpal yash-rajpal requested a review from a team as a code owner July 4, 2024 13:36
Copy link
Member

@gabriellsh gabriellsh left a comment

Choose a reason for hiding this comment

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

The tests you wrote don't actually test the issue that is being fixed. It is a nice addition, but we need an end-to-end test for this, or:

  • Extract the header items filter logic to a hook and unit test that hook
  • Unit test RoomToolboxE2EESetup component

One of these 3 should be enough for this

Copy link
Member

@gabriellsh gabriellsh left a comment

Choose a reason for hiding this comment

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

Left some comments. Overall I'm wondering on the complexity of these tests, if it makes sense to implement all those checks on the same test evaluation.

In general I'd like to see more atomic tests. For ex: in this case you're testing the header buttons visibility, but there's a lot of other components of the feature being tested. I like to see that you included tests for the whole UX for the save password state, but it could be a little more organized.

What I mean is, a describe for this whole flow, and then a test clause for each behaviour you're testing.

Please let me know if you wanna discuss any of these topics or if you disagree with anything. All feedback is welcome.

apps/meteor/tests/e2e/e2e-encryption.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/e2e-encryption.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/e2e-encryption.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/e2e-encryption.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/e2e-encryption.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/e2e-encryption.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/e2e-encryption.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/e2e-encryption.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/e2e-encryption.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/e2e-encryption.spec.ts Outdated Show resolved Hide resolved
@yash-rajpal
Copy link
Member Author

Implemented a E2E test to cover this case, but for some reason the test is always failing on CI, so removed the test from this PR, will implement the test in a separate PR and it would be tracked by this task

@kodiakhq kodiakhq bot merged commit 1eadbe6 into release-6.10.0 Jul 5, 2024
42 checks passed
@kodiakhq kodiakhq bot deleted the reg/e2ee-dm-setup-header branch July 5, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants