Change how ReplyWidget style is updated on changes #831
Conversation
rmol
requested review from
sssoleileraaa,
kushaldas and
redshiftzero
as code owners
|
Were you able to determine why we've only been able to reproduce #825 in nightlies & prod builds, and when it was introduced? |
|
I was able to reproduce it locally by adding the timeout as in the test plan. I don't think there was anything introduced that caused this. As @redshiftzero noted it could be reproduced with a much earlier version of the client. It's just that we've not had any handling of the case where the client has decided that five attempts to send the reply have timed out, when they actually succeeded, creating a reply on the server that got retrieved during sync. |
|
I still find it very strange that all replies suddenly started exhibiting this behavior, but only in nightlies. |
|
Per a test run I did over the weekend this does not resolve #825 in the packaged build, see report in #825 (comment) |
rmol
changed the title
|
i just ran through the test plan and i see that this resolves #825 so all that's left is code review |
Simply setting the stylesheet on ReplyWidget has stopped working; styles are not being applied to children. My working theory is that it might be due to changes in the Debian package qtbase-opensource-src at 5.11.3+dfsg1-1+deb10u2, which incorporated upstream changes that changed how stylesheets are applied to children. That theory is a bit shaky, though, because with the same system packages installed, installing the same version of PyQt5 in a virtualenv results in the old approach working fine. That's unfortunately not an option for us at this time, as it would be a huge addition to the production requirements, and potentially painful to keep in sync with the system Qt packages. At any rate, the workaround in this change, to explicitly apply the CSS to the children, seems to solve the problem of reply widgets not being updated correctly for their status.
sssoleileraaa
force-pushed
the
825-apprise-replies
branch
from
8cf6124
to
35b2d46
Compare
| @@ -1786,6 +1786,25 @@ def _on_reply_failure(self, message_id: str) -> None: | |||
| if message_id == self.message_id: | |||
| self._set_reply_state('FAILED') | |||
|
|
|||
| @pyqtSlot(str, str, str) | |||
| def _update_text(self, source_id: str, message_id: str, text: str) -> None: | |||
There was a problem hiding this comment.
I think we should discuss this change that you added - this is not necessary in order to fix #825 and I believe it complicates the code unnecessarily. The issue you refer to is already resovled in the SendReplyJob because it will know whether or not it gets a response back from the server. If it times out the job will always be readded to the queue and the next time around it will see that the reply was actually sent because it'll exist in the database.
This change doesn't break anything and everything else looks good. I did regression testing around replies and their different states. Since we want to see the nightly succeed before cutting a release tomorrow, I'm going to go ahead and merge and we can discuss this after standup.
There was a problem hiding this comment.
We don't need to discuss it. If you've fixed it elsewhere and it's superfluous it should be removed.
Description
Sets ReplyWidget state to succeeded when its text is updated. Apply CSS to its children according to its state.
This solves the problem of replies being stuck in pending state if their uploads only seemed to the client to fail, but did in fact succeed.
It also works around a new failure in Qt stylesheet handling which might be due to changes in the Debian package
qtbase-opensource-srcat5.11.3+dfsg1-1+deb10u2, which incorporated upstream changes that changed how stylesheets are applied to children.That theory is a bit shaky, though, because with the same system packages installed, installing the same version of PyQt5 in a virtualenv results in the old approach working fine. That's unfortunately not an option for us at this time, as it would be a huge addition to the production requirements, and potentially painful to keep in sync with the system Qt packages.
Fixes #825.
Test Plan
To test the stylesheet failures:
sd-app.To test the reply timeout scenario:
at line 232 of
securedrop/journalist_app/api.py(right afterelif request.method == 'POST':). This will ensure that the client's reply jobs will time out.make devsecuredrop-clientrepo, on the master branch, start the client withrun.sh.Failed to send.Checklist
If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:
If these changes add or remove files other than client code, packaging logic (e.g., the AppArmor profile) may need to be updated. Please check as applicable: